Opened 5 years ago

Closed 5 years ago

#60403 closed defect (fixed)

lpcnetfreedv: incorrect optimization when building universal

Reported by: ryandesign (Ryan Carsten Schmidt) Owned by: ra1nb0w
Priority: Normal Milestone:
Component: ports Version: 2.6.2
Keywords: Cc:
Port: lpcnetfreedv

Description

The lpcnetfreedv portfile contains this block:

pre-configure {
    # enable optimization on all Intel hardwares
    if {${build_arch} eq "i386"} {
        configure.args-append \
            -DCMAKE_C_FLAGS="-mssse3 -msse4.1"
    } elseif {${build_arch} eq "x86_64"} {
        configure.args-append \
            -DAVX=ON
    }
}

This will not be correct when building with the universal variant, because you will be basing the optimization choice on the arch of the build machine rather than choosing it correctly for each arch that you're building. If you need to use different flags for each arch, as appears to be the case here, you'll want to investigate the muniversal portgroup.

As an aside, you don't really ever want to do configure.args-append -DCMAKE_C_FLAGS=.... Instead, you'll want to do configure.cflags-append ... and let the cmake portgroup handle how that gets communicated to cmake.

Also, if you're going to check whether a variable equals one value, and if not then check if it equals another value, etc., the proper construct is not a chain of if statements but rather a switch statement.

Change History (4)

comment:1 Changed 5 years ago by ra1nb0w

Sorry for the delay.

this is the change https://github.com/ra1nb0w/macports-ports/commit/4c9b9c99c28ec74677a8d8faaacc30e1b4557ce2

is it ok?

thanks for the tip about the switch; generally I use if/else(if) when I have only a few checks because they are generally faster or the compiler optimize the switch to if/else(if). Surelly the readability is better with switch.

comment:2 Changed 5 years ago by ryandesign (Ryan Carsten Schmidt)

Thanks. I haven't tested it but it looks like that should work.

The only other thing I'm wondering about is that right before the block you added, there's this:

# disable AVX and AVX2 for compatibility
configure.args-append \
    -DDISABLE_CPU_OPTIMIZATION=ON

And right after your block, there's this:

# select native cpu flags
variant native description {Enable auto selection of cpu flags like avx/avx2/neon} {
    configure.args-delete -DDISABLE_CPU_OPTIMIZATION=ON
}

Is the optimization that's enabled by your -DAVX=ON different from the AVX and AVX2 flags mentioned in this existing +native variant?

comment:3 Changed 5 years ago by ra1nb0w

Yes, they are different. with DISABLE_CPU_OPTIMIZATION=OFF cmake try to discover the best options available; therefore can be: AVX, AVX2 and/or AVX512.

Ok. I merge. Thank you!

comment:4 Changed 5 years ago by Davide Gerhard <ra1nb0w@…>

Resolution: fixed
Status: assignedclosed

In 67a07f50f640da74ba883ce7ef322f040aeb4047/macports-ports (master):

lpcnetfreedv: fix universal support

Closes: #60403

Note: See TracTickets for help on using tickets.