LGTM.

This doesn't add metadata to llvm intrinsics that are not constrained.

Oh, right. I misunderstood what's doing in these patches and thought we can add metadata to any intrinsics by CGFPOptionsRAII now. :-)

If a relevant #pragma is used then without this change the metadata ...

Yeah, I understand it now. Thank you! But why we still have the wrong maytrap with this patch?

It's true that the middle-end optimizers know nothing about the constrained intrinsics. Today. I plan on changing that in the near future.

Brilliant!

If use of the constrained intrinsics will cause breakage of the target-specific clang builtins then that's important to know and to fix.

I had a look at these changes and didn't find anything will cause breakage for now. I'm still not sure if we need to teach target-specific clang builtins to respect strict FP or not. But it has nothing to do with this patch.

What is the reason for treating this differently in LLVM?

tile config register is the user of each AMX instruction

Hi Kevin, what's the intention of adding constrained FP metadata for target dependent builtins? I believe the middle-end passes always ignore these builtins. What's more, will it imply user these builtins have different behaviors under different FP model? But it's not true for most platform builtins, since they are just representative of given instructions.

LGTM, but I'd like to see if @lebedev.ri has any objections.

to amx intrinsics the the

In D94268#2487765, @jyknight wrote:

In D94268#2485958, @pengfei wrote:

Is inline assembly the only case emms instruction will be needed? But inline assembly doesn't enable mmx attribute automatically, right? E.g. https://godbolt.org/z/43ases

Yes, inline or external asm should be the only reason there should be any MMX register usage when all is done here. After this patch, the default is still to have mmx enabled by default with sse, despite that clang won't use it. But users can pass -mno-mmx if they like. And, yes, clang only requires -mmmx if you use the "y" asm constraint, not if you use mmx instructions inside the asm string.

I wrote this patch because making it a no-op is the same behavior GCC has. However, I'm not sure this is necessarily the right way to go. On the plus side for this patch, it allows intrinsic-using code to stop emitting spurious emms instructions, if compiled with -mno-mmx. However, the negative is that inline-asm code which _doesn't_ use the "y" constraint might still be using MMX within an asm blob, and be depending on calls to _mm_empty() outside of the asm, and such code would be silently broken when compiled with -mno-mmx.

At first, I thought that most uses of inline-asm would be using constraints, but after looking around at existing MMX asm, it seems that nearly all of it does _not_ use a "y" constraint or even clobber any fpu or mmx registers. And they do also depend on _mm_empty() in combination with their unmarked inline asm. Which...now that I think about it more, makes passing -mno-mmx to the compiler almost entirely pointless.

So, now I'm thinking I'll just drop this change, actually.

Is inline assembly the only case emms instruction will be needed? But inline assembly doesn't enable mmx attribute automatically, right? E.g. https://godbolt.org/z/43ases
Analyzing asm block and appending the mmx attribute if we see mmx instructions might be needed. But if we do the analysis, just adding an emms instruction at the end of the block seems better.

LGTM.

I saw the document mentions ESP/RSP should be preserve for regcall. But I cannot think out a situation that ESP/RSP need to preserve either.

LGTM.

LGTM.

Let me know if I am not answering/understanding the questions.

In D93860#2475028, @spatel wrote:

In D93860#2473348, @pengfei wrote:

Hi Sanjay, pairwise reduction seems not adopted by any targets. But do we need to consider the FMF here? I found LangRef says the intrinsic is sequential for fadd and fmul if reassoc flag is not set.

Hi @pengfei -
Sorry for the delayed reply - I did not get an email notification for your comment here in Phab.

We do not need to care about FMF directly here because neither of these shuffle variations corresponds to "sequential" reduction of the elements. In other words, we should require reassoc (at the least) to form an fadd or fmul reduction that is expected to need any shuffles. Let me know if you see any holes in that theory.

One motivation for this cleanup is to correct bugs with using/propagating FMF (that was also true for D87416). For example - https://llvm.org/PR35538

In D93179#2473620, @RKSimon wrote:

In D93179#2473342, @pengfei wrote:

Hi Simon, I found we have the same problem for fadd/fmul. See https://godbolt.org/z/3YKaGx
X86 Intrinsics imply reassoc flag, but llvm.vector.reduce.* doesn't.

I'm not surprised - my current plan (after the holidays) is to add doxygen descriptions for all the reduction intrinsics and then update them making it clear what fast-math flags are assumed.

Hi Sanjay, pairwise reduction seems not adopted by any targets. But do we need to consider the FMF here? I found LangRef says the intrinsic is sequential for fadd and fmul if reassoc flag is not set.

Hi Simon, I found we have the same problem for fadd/fmul. See https://godbolt.org/z/3YKaGx
X86 Intrinsics imply reassoc flag, but llvm.vector.reduce.* doesn't.

LGTM. Thanks for the refactors. Maybe better to wait for a few days to see if others have objections.

LGTM.

In D91927#2470818, @LuoYuanke wrote:

In D91927#2469977, @pengfei wrote:

In my test case, it is transformed after Combine redundant instructions.

Can we disable it for AMX type? The pointer to AMX type is meaningless and may result in bad perfomance.

Ok, I'll disable the transform for AMX type.

In my test case, it is transformed after Combine redundant instructions.

LGTM.

LGTM.

LGTM.

LGTM. Thanks.
update_test_prefix.py assumes the conflicting output. You may need to change the expection of it as well.

LGTM.

In D93179#2452932, @spatel wrote:

If we're going by existing behavior/compatibility, gcc/icc use packed ops too:
https://godbolt.org/z/9jEhaW
...so there's an implicit 'nnan nsz' in these intrinsics (and that should be documented in the header file (and file a bug for Intel's page at https://software.intel.com/sites/landingpage/IntrinsicsGuide/ ?).

What's the difference with the existing code? It looks to me that you just brought the warning out of loop, right?

Yes, it is using maxpd/minpd here. Sorry for missing the nan cases.
In fact, I doubt about the behavior when using fast math with target intrinsics. In my opinion, target intrinsics are always associated with given instructions (reduce* are exceptions). So the behavior of intrinsics, e.g. respect nans, signaling, rounding, exception etc. are concordance with their associated instructions. That said the fast math flags won't change the behavior of intrinsics.
Assume above, I'm happy to set these expansions to fast math. Either keeping the existing implementaion or expansion LGTM.

LGTM. Thanks for bringing this refactor.
I also verified that ICC and GCC both do reduce math in an binary tree way, though sometimes ICC has a different LSB from GCC and Clang.

looks like quite a few (3K) of tests in CodeGen (14K or so) use it. So I wanted to understand why removing a prefix used nowhere in the test lead to an incorrect rewrite of some assertions.

There are more then 6K tests. update_llc_test_checks.py is just one of the update scripts.

Add initial author to have a review.

I think you meant --allow-unused-prefixes=true.

Yes, that's what I meant. Thanks for correct me.

In D92965#2444565, @mtrofin wrote:

In D92965#2444553, @pengfei wrote:

I left about 12 tests when I fixed the prefixes issues using the script, because the changes on prefixes will resulted in conflict when re-generating them. I suspected if it is a bug in update_llc_test_checks.py, but I didn't find time to look through. Maybe we can add --allow-unused-prefixes=false for these failed tests as a workaround?

I actually just undid the re-generation changes, leaving just the unused prefix removal. I'm not sure what the pros/cons to re-generation would be - my reasoning was about "keeping this change scoped". The tests passed, though with a FileCheck with the flag flipped to not allow unused prefixes.

LGTM. I think we can land this patch as a beginning. Cheers~

I left about 12 tests when I fixed the prefixes issues using the script, because the changes on prefixes will resulted in conflict when re-generating them. I suspected if it is a bug in update_llc_test_checks.py, but I didn't find time to look through. Maybe we can add --allow-unused-prefixes=true for these failed tests as a workaround?

The changes on the tests look good to me. But I'm not sure the changes on the code. I'd like to see opinions from other reviewers.

LGTM.

Cheers~

Glad to see you are using the script. I'm not very sure its behavior. Please pay attention to its changes especially marked CHANGE

It's better to set parent since the patch is based on D91927.

LGTM.

In D91338#2421551, @hvdijk wrote:

@pengfei With the confirmation that we do need this, could you take another look? Does this look okay now?

Fix a missed change.

Adress Craig's comments.

Revert unrelated change.

LGTM. You'd better wait few days to see if others object. Thanks.

Can you add more detail for the background on summary?

In D80344#2407305, @tentzen wrote:

In D80344#2407250, @pengfei wrote:

Do we need to consider FP exceptions in _try block?

Yes, FP exception is handled as long as FP exceptions are not disabled (Ex via _controlfp() runtime) and FP exception code is filtered & handled via ___except() statement (Ex, ___except(GetExceptionCode()==EXCEPTION_FLT_INEXACT_RESULT)).