I'll re-work this to handle it in the lowering, as @lebedev.ri suggested. And take the other comments into account then.

Sounds like the lowering should just be improved to do the opposite of this?

I intend to commit an NFC version of the updated test that shows the current behavior of main, with TODOs in it indicating the desired changes.

LGTM

Thanks for the quick reviews!

Thanks Sanjay!

In D129523#3655223, @spatel wrote:

This is a straightforward translation of the existing FMF checks, so LGTM.
I added some inline comments for potential cleanups that could be done as NFC commits either before or after.

Update to:

1. Remove the cleanup that has been done separately (and committed as 230c8c56f21cfe4e23a24793f3137add1af1d1f0).
2. Check only for reassoc and nsz (rather than isFast()).

Committed at 230c8c56f21cfe4e23a24793f3137add1af1d1f0.

Added an integer version of a reassociation test, including a TODO of a possible further improvement.

In D129612#3652041, @spatel wrote:

I'd prefer to have that test, so we have coverage for the integer path added with this patch. It's still an improvement even if it's not as big as the FP side.
Other than that, LGTM.

Addressed review comments from @spatel , except for adding an integer test.

In D129612#3648882, @spatel wrote:

Is it possible to create an integer (mul and sub) test that also has this problem?

To add some context to make it easier to review, the approach taken here of adding a ReassociatePass::OrderedSet parameter (named ToRedo) to the function LinearizeExprTree , is the same technique already used in two other functions in "Reassociate.cpp": NegateValue and BreakUpSubtract.

I'll split this into two independent patches.

I've split off the miscellaneous cleanup into a separate patch: D129612. Once that wraps up, I'll update the patch here to just be the improved optimization with reassoc and nsz.

Thanks for the quick feedback!

Assuming this approach is sensible, I'm raising the question of whether this patch should be split into two.

Thanks for the review @RKSimon !

Clear simple, safe fix. And I've verified it fixes the problem we ran into downstream.
LGTM

LGTM!

This commit appears to be the root cause of a run-time crash related to the interaction of global initializers and the wrapper functions to access thread_local variables -- reported as PR48030.

LGTM

In D86907#2253599, @Sunil_Srivastava wrote:

Tweaked the test to specifically look for the error of interest

In D86907#2250689, @Sunil_Srivastava wrote:

Simplified the test case.

Revisiting this patch. I think that before fixing the -ffp-contract=off problem I originally raised here, there are two questions that have come up in this discussion that we should first resolve. Specifically:

LGTM

Used the clearer '!off && !on' (rather than '!(off || on)') in the checks.

About:

This is a bit of an oddity in our handling.

Yes it is!

This is certainly getting more complicated than I had originally expected. I feel there isn't a clear spec on what we want in terms of whether FMA should be enabled "automatically" at (for example) '-O2', and/or whether it should be enabled by -ffast-math. I'm very willing to make whatever change is needed here, to meet whatever spec we all ultimately agree on.

So I think this patch should be put on hold until we decide on these wider aspects.

Update a comment to remove the discussion about enabling the __FAST_MATH__ preprocessor macro. The handling of the setting of __FAST_MATH__ is done in "clang/lib/Frontend/InitPreprocessor.cpp", and once we decide on the set of conditions that enable that definition, we can add an appropriate comment there.

In D72675#1839662, @andrew.w.kaylor wrote:

In D72675#1839492, @wristow wrote:

1. Should we enable FMA "by default" at (for example) '-O2'?

We recently introduced a new option "-ffp-model=[precise|fast|strict], which is supposed to serve as an umbrella option for the most common combination of options. I think our default behavior should be equivalent to -ffp-model=precise, which enables contraction. It currently seems to enable -ffp-contract=fast in the precise model (https://godbolt.org/z/c6w8mJ). I'm not sure that's what it should be doing, but whatever the precise model does should be our default.

A separate question is the interaction of -ffast-math with -ffp-contract=. Currently, there is no such interaction whatsoever in GCC: -ffast-math does not imply any particular -ffp-contract= setting, and vice versa the -ffp-contract= setting is not considered at all when defining __FAST_MATH__. This seems at least internally consistent.

Updated patch to correct a comment and fix a typo.

Jinx ;) This took quite some tracking down on our side - too bad we didn't wait a couple of weeks.

Changing this to address only the Clang driver aspect, as discussed in the comments. Will spin off a separate patch for the LLVM side.

One commit for the clang changes should be ok; it's a very small diff. But I'm still not sure if the driver change induces frontend diffs that we should make visible via tests.

The only thing I can think of is that it changes whether/when __FAST_MATH__ is defined. But that'll be indirectly tested, via the updated tests in "Driver/fast-math.c", which will verify that "-ffast-math" is passed appropriately (and the __FAST_MATH__ dependency on "-ffast-math" is already tested in "Preprocessor/predefined-macros.c").

This all sounds good to me.

This is a follow up to https://reviews.llvm.org/D72529.

Independently, we came across this same problem, and I proposed a patch at D72469 (which has been committed) that is essentially the simple fix that is described above:

Addressed comments from @hfinkel .

Thanks for the quick feedback @hfinkel

In D72469#1819623, @probinson wrote:

Seems to me we've needed to do this sort of tweak before, so LGTM.
Because it depends on the memory-management behavior of the map, it would be tricky to test with IR input. Would it be feasible to do something in llvm/unittests/ADT/SCCIteratorTest.cpp that could trigger this reliably? Can be done as a follow-up, as I know the branch is coming soon.

Thanks for the quick review.

Given the performance improvements here, I'd like to develop this patch further.

In D69732#1784290, @ormris wrote:
Ping @tejohnson

This probably needs to be taken over by someone who cares about full LTO performance

In D64780#1593624, @aaron.ballman wrote:

LGTM aside from a nit. You should give other reviewers a chance to sign off in case they have additional comments.

Do we need to keep the old behavior on platforms where clang is the de facto compiler?

Updated patch to address comments from @RKSimon cand @fhahn -- primarily change the default so that nontemporal misaligned mem-ops are assumed to not exist.

Thanks @fhahn. I'll update the patch to address those comments.

This patch was accepted by @andreadb about a week ago, but Andrea said he'd leave the final decision to @RKSimon. With Simon's comments from a week ago asking @fhahn if he had any preferences for keeping the current default as-is, or changing it to something that seems to reflect existing hardware better, I've been hesitant to actually commit it, even though it's marked as Accepted. With a week now gone by, I'll plan on committing this tomorrow, unless Simon or Florian raise any concerns.

In D61764#1519660, @wristow wrote:

In D61764#1517571, @RKSimon wrote:

Would it be possible to add tests where non-temporal load/stores successfully vectorize?

Glad to see your comment about SSE4A supporting nt-stores at any alignment. With that, I can make an X86 test-case that does vectorize.

Updated the patch to allow arbitrary alignment of float and double nt-stores for SSE4A.

In D61764#1517571, @RKSimon wrote:

Would it be possible to add tests where non-temporal load/stores successfully vectorize?

ping

One point I want to explicitly raise, is that prior to this proposed change, the current vectorization implementation ignores the nontemporal hint when checking whether a memory op is vectorizable. So conceptually, it's as though the two new routines:
bool isLegalNTStore(Type *DataType, unsigned Alignment)
bool isLegalNTLoad(Type *DataType, unsigned Alignment)
return true for all types and alignments. With that in mind, I've made the (default) target-independent implementations of these functions return true -- it's only the overriding functions on X86 (which is the target PR40759 was reported against) that will ever return false here (so on non X86 targets, this would be an NFC commit). But in looking through the other backends, I believe that none of them support misaligned nontemporal mem ops. So possibly a better default would be to have these new functions return true only if the specified DataType is minimally aligned at Alignment.