Fix the AVX2 backend build

[dkostic]

Summary:
For reasons that elude me, this fixed issue #574 on my machine.

Steps:
If your pull request consists of multiple sequential changes, please describe them here:

Performed local tests:

• lint passing
• tests all passing
• tests bench passing
• tests cbmc passing

Do you expect this change to impact performance: Yes/No

If yes, please provide local benchmarking results.

[hanno-becker]

Unfortunately we can't do that without reverting also the change removing EXTRAFLAGS at the end: We want user specified CFLAGS do overwrite default CFLAGS, but with this version it's the other way around

[hanno-becker]

The issue is lazy vs. eager evaluation of ARCH_FLAGS within CFLAGS: with := the variable is eagerly evaluated at time of definition, while with += it is lazily evaluated. This matters since auto.mk sets default ARCH_FLAGS only later based on your system.

Some options:

1. Just move the inclusion of auto.mk upwards before the setting of CFLAGS.
2. Modify CFLAGS, not ARCH_FLAGS, in auto.mk. We still need to keep ARCH_FLAGS in the CFLAGS defaults though because some test scripts set it, too.
3. Make a copy of the original CFLAGS as (say) ORIG_CFLAGS := $CFLAGS and switch back to lazy evaluation for CFLAGS, but using ORIG_CFLAGS in the end.

If it works, 2. would be simplest. Can you try @dkostic ?

[hanno-becker]

Thanks a lot @dkostic for spotting this. make apparently eludes me, too.

See the comment for what happened. For the fix we can't use the proposed change as we want to give the original CFLAGS precedence. It seems simplest to have auto.mk amend CFLAGS, not ARCH_FLAGS. The distinction is anyway not a technical one. Could you please also adjust the CI tests to pass FORCE_AARCH64 and FORCE_X86_64 accordingly, so this would have been caught?

Before we can consider merging, we need to move this PR to a non-remote branch -- this is because of some peculiarities about our EC2 CI still... (sorry!) @mkannwischer can you either give @dkostic developer rights so he can push branches, or otherwise run CI yourself when things look final?

[mkannwischer]

Before we can consider merging, we need to move this PR to a non-remote branch -- this is because of some peculiarities about our EC2 CI still... (sorry!) @mkannwischer can you either give @dkostic developer rights so he can push branches, or otherwise run CI yourself when things look final?

Sorting out access during the holiday's is likely going to be impossible as it need approval in the TSC repository. Happy to merge this into a local branch first to run the full CI.

[mkannwischer]

The issue is lazy vs. eager evaluation of ARCH_FLAGS within CFLAGS: with := the variable is eagerly evaluated at time of definition, while with += it is lazily evaluated. This matters since auto.mk sets default ARCH_FLAGS only later based on your system.

Some options:

1. Just move the inclusion of auto.mk upwards before the setting of CFLAGS.
2. Modify CFLAGS, not ARCH_FLAGS, in auto.mk. We still need to keep ARCH_FLAGS in the CFLAGS defaults though because some test scripts set it, too.
3. Make a copy of the original CFLAGS as (say) ORIG_CFLAGS := $CFLAGS and switch back to lazy evaluation for CFLAGS, but using ORIG_CFLAGS in the end.

If it works, 2. would be simplest. Can you try @dkostic ?

I implemented (2) in #578 - let's get that merged quick and discuss any further improvment separatelty.

[hanno-becker]

Thanks @mkannwischer!

With this merged, how should this be detected in the future? Either of (a) not setting arch flags in CI, but relying on defaults, or (b) passing FORCE_xxx would have caught this -- we should probably do both. Note though that even with (a), what we benchmark is not the default build, because the latter disables LTO.

[mkannwischer]

Superseded by #578

[mkannwischer]

Thanks @mkannwischer!

With this merged, how should this be detected in the future? Either of (a) not setting arch flags in CI, but relying on defaults, or (b) passing FORCE_xxx would have caught this -- we should probably do both. Note though that even with (a), what we benchmark is not the default build, because the latter disables LTO.

Let's do both.