Thanks for your review. Hope this patch won't cause too many ABI issues in the future.

Address Craig's comments

Address Simon's comments

In D100368#2685189, @RKSimon wrote:

Add _mm512_i32loscatter_epi64 and _mm512_mask_i32loscatter_epi64 for completeness?

Address Craig's comments

In D99565#2678073, @craig.topper wrote:

I think I wouldn't mind if we just didn't emit aligned loads/store instructions for AVX/AVX512 from isel and other places in the compiler in the first place. As noted, if the load gets folded the alignment check doesn't happen. That would reduce the size of the isel tables and remove branches, reducing complexity of the compiler. Adding a new step and a command line to undo the earlier decision increases complexity.

The counter argument to that is that the alignment check has found bugs in the vectorizer on more than one occasion that I know of.

In D99565#2682809, @lebedev.ri wrote:

I'm still uncomfortable with changing current status quo, even though i obviously don't get to cast the final vote here.

One should not use aligned loads in hope that they will cause an exception to detect address misalignment.
That's UBSan's job. -fsanitize=undefined/-fsanitize=aligment *should* catch it.
If it does not do so in your particular case, please file a bug, i would like to take a look.

Likewise, i don't think one should do overaligned loads and hope that they will just work.
UB is UB. The code will still be miscompiled, but you've just hidden your warning.

Likewise, even if unaligned loads can be always used, i would personally find it pretty surprising
to suddenly see unaliged loads instead of aligned ones.
Also, isn't that only possible/so when AVX is available?
Also, doesn't that cause compiler lock-in?
What happens without AVX? Do so anyways at the perfomance's cost?
Or back to exceptions?

Should this process in any form other than the UBSan changes,
i would like to first see a RFC on llvm-dev.
Sorry about being uneasy about this. :S

1. Rebase;
2. Emit unaligned move in ISEL;
3. Only do the conversion on AVX machine.

In D99565#2678073, @craig.topper wrote:

I think I wouldn't mind if we just didn't emit aligned loads/store instructions for AVX/AVX512 from isel and other places in the compiler in the first place. As noted, if the load gets folded the alignment check doesn't happen. That would reduce the size of the isel tables and remove branches, reducing complexity of the compiler. Adding a new step and a command line to undo the earlier decision increases complexity.

The counter argument to that is that the alignment check has found bugs in the vectorizer on more than one occasion that I know of.

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

In D100091#2675968, @craig.topper wrote:

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

Address Pengfei's comments

Ping?

Rebase and avoid using 'byval' parameter.

Address pengfei's comments

In D98011#2610944, @pengfei wrote:

In D98011#2610940, @lebedev.ri wrote:

In D98011#2610939, @pengfei wrote:

In D98011#2610900, @lebedev.ri wrote:

In D98011#2610381, @LiuChen3 wrote:

In D98011#2605726, @lebedev.ri wrote:

Tests, and some words in description/patch, are missing.

Thanks for your review, @lebedev.ri. This is actually one NFC patch. I think we don't need add new tests.

How is it NFC?

I think @LiuChen3 's NFC firstly means to LLVM. I.e. Moving the manually added check to .inc that generated by tablegen. It's hard to say NFC for the tablegen code. But with a coarse search, I didn't find we have a test for this EVEX2VEX backend. So I assumed it is covered by LLVM tests. By this mean, I think we can call it a NFC :)

That should be explained in patch's description :)

Agreed. Thanks.

Address comments

Address pengfei's comment.

Rebase and address Pengfei and Craig's comments.

In D98011#2605726, @lebedev.ri wrote:

Tests, and some words in description/patch, are missing.

I don't know why pre-merge-checks failed. I can check-all successfully locally in redhat8. I don't have debian mainchine to reproduce this problem.

Address Pengfei and Yuanke's comments. We don't need more tile type.

Adding back 'avx512f' to amx-tile-basic.ll

The above threshold is for number of MIs. BB->size() is to get instruction number of BB. I committed 31c2b93d83f63ce7f9bb4977f58de2e00bf18e0f to further reduce compiling time. You can have a try

In D86864#2472375, @shchenz wrote:

In D86864#2472369, @LiuChen3 wrote:

I can surely do that. But I think the most reasonable solution would be fix the compiling time issue. Since compiling time tests I did before does not expose any regression, your test case must be a little special. Could you find out the special point, for example the function has too many blocks or some/many blocks in the function has too many instructions? Thanks.

I think the increase in compile time is because the function has too many instructions and blocks. The function has Thousands of lines of instructions. Can you add limitation for the number of instructions or the number of blocks so the check for 'store' can end early?

Could you please help to check if the following change can solve your issue?

C
diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index 0abdf89..8ca3520 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -79,6 +79,12 @@ static cl::opt<unsigned> SplitEdgeProbabilityThreshold(
         "splitted critical edge"),
     cl::init(40), cl::Hidden);
 
+static cl::opt<unsigned> SinkLoadInstsPerBlockThreshold(
+    "machine-sink-load-instrs-threshold",
+    cl::desc("Do not try to find alias store for a load if there is a in-path "
+             "block whose instruction number is higher than this threshold."),
+    cl::init(2000), cl::Hidden);
+
 STATISTIC(NumSunk,      "Number of machine instructions sunk");
 STATISTIC(NumSplit,     "Number of critical edges split");
 STATISTIC(NumCoalesces, "Number of copies coalesced");
@@ -1036,6 +1042,12 @@ bool MachineSinking::hasStoreBetween(MachineBasicBlock *From,
     HandledBlocks.insert(BB);
     // To post dominates BB, it must be a path from block From.
     if (PDT->dominates(To, BB)) {
+      // If this BB is too big, stop searching to save compiling time.
+      if (BB->size() > SinkLoadInstsPerBlockThreshold) {
+        HasStoreCache[BlockPair] = true;
+        return true;
+      }
+
       for (MachineInstr &I : *BB) {
         // Treat as alias conservatively for a call or an ordered memory
         // operation.

I can surely do that. But I think the most reasonable solution would be fix the compiling time issue. Since compiling time tests I did before does not expose any regression, your test case must be a little special. Could you find out the special point, for example the function has too many blocks or some/many blocks in the function has too many instructions? Thanks.

Yes, we have been aware that this patch may introduce compiling time degradations. And as you can see in previous comments, I already tested the compiling time on X86 arch. Sadly, the tested benchmarks don't expose any regressions.

Could you please help to send me your regression function/IR? So I can have a look about how to fix it? Thanks.

Hi, @shchenz. Our several opecncl benchmarks have appeared great compile time regression.
For only one function, the time consume on Machine code sinking pass increased form 6.0711s to 366.5713.
According to your algorithm, this patch will obviously increase the compile time for some cases.

In D78564#2459608, @hjl.tools wrote:

In D78564#2459571, @LiuChen3 wrote:

In D78564#2457869, @hjl.tools wrote:

Does this fix https://github.com/golang/go/issues/42440?

No. I think this patch can only fix part of the issue.

Can you fix the go issue?

In D78564#2457869, @hjl.tools wrote:

Does this fix https://github.com/golang/go/issues/42440?

It allows more than two, right? like {vex}{vex2}{vex3} instruction. I think it should be a bug for att.

2. Delete IsPrefix parameter, and delete 'break'

1. Check prefix, ignoring case
2. Delete IsPrefix parameter, and delete 'break', so that we won't check prefix again. I am not sure if this is right. Att format can allow two prefix and using the last one as the finally encoding prefix. I think this may not be the original intention of the design.
3. Change the test: checking the IR istead of checking the assembly.
4. Made some format adjustments.

Ping?

Rebase.
Adding the '{}' to prefix when generate IR.

1. Address comments;
2. Only support parsing vex/vex2/vex3/evex prefix for MASM

Thanks for all of your review!

In D89105#2363927, @pengfei wrote:

LGTM. Thanks. Better to wait one day or two to see if others object.

1. Move the testcase from avx-vnni/ to test/CodeGen/X86/.
2. Refine the Run line in avx_vnni-intrinsics.ll

Address comments

In D89184#2363591, @echristo wrote:

Let's go ahead and unblock you, but getting a lot of this refactored would be great if you can. I think it's hitting the limits of the original design. :)

In D89184#2360846, @echristo wrote:

I'll take a look tomorrow, sorry for the delay.

Ping?

In D90009#2354849, @kiranchandramohan wrote:

I have added a fix to run the test only when the X86 target is available. Please feel free to change if it is not the correct fix.
https://github.com/llvm/llvm-project/commit/c551ba0e90bd2b49ef501d591f8362ba44e5484d

Address comments.

Adding avxvnni to Alder Lake.

1. move the commonvnniintrin.h to the avx512vlvnniintrin.h.
2. move the testcase avxvnni-builtins.c to X86 subdirectory.