In D105889#4334504, @paulwalker-arm wrote:

In D105889#4323865, @TiehuZhang wrote:

Hi, @bsmith, sorry to bother you, could you tell me the background of the patch?
I have a question about the patch, as I notice that the patch adds movprfx before some instructions, and the optimization may affect the performance(degradation).
; CHECK-NEXT: ptrue p0.d
+; CHECK-NEXT: movprfx z0, z1
; CHECK-NEXT: fsqrt z0.d, p0/m, z1.d

The rational for the patch is to ensure the inactive lanes of unary operations don't have a dependence on previous operations when the results for inactive lanes is not important (i.e. undef). Taking the above example, the problem with not breaking the dependency is that if z0.d is used as the result for a preceding long latency instruction then it can negatively impact the issue latency of this instructions despite there being no "real" dependency.

For this reason it is preferred to always break such dependencies via a movprfx instruction. The intent here is that movprfx is suppose to be free because the architecture prefers implementations to combine movprfx'd destructive instructions into constructive instructions. However, we are aware of issues where register allocation is making some poor decisions that is leading to movprfx being over used because dedicated destination registers are being allocated even though it would be advantageous to reuse an operand register.

Hi, @bsmith, sorry to bother you, could you tell me the background of the patch?
I have a question about the patch, as I notice that the patch adds movprfx before some instructions, and the optimization may affect the performance(degradation).
; CHECK-NEXT: ptrue p0.d
+; CHECK-NEXT: movprfx z0, z1
; CHECK-NEXT: fsqrt z0.d, p0/m, z1.d

In D141590#4051555, @lebedev.ri wrote:

Yes, of course running more passes helps with optimizations. That is obvious and was not my question.
My question is why we need to do that in the first place, and why we can not catch those cases during the existing pass runs.

In D141590#4047397, @lebedev.ri wrote:

I'm also a bit skeptical about this change.
I would really recommend to fix the LoopIdiom pass itself,
or at least explaining why that is impossible/unfeasible.

In D141590#4046933, @fhahn wrote:

Could you share a full IR test showing the improvements? The main goal of the extra vectorizer passes is to optimize the vector code, not necessarily other scalar loops.

The patch may be helpful for generating better IR in some cases, e.g.,

void testNestLoop(const int nCells, const double *x, double *b, const double *values, const int max_row_length){

Update the test:

1. Supplement other data types (nxv2i64, nxv4i32 and nxv8i16).
2. Add functions with different operand orders.

In D127720#3743340, @housel wrote:

In D127720#3741600, @TiehuZhang wrote:

In D127720#3741484, @housel wrote:

In D127720#3741401, @TiehuZhang wrote:

Hi, @housel, the following error occurs when I run the test suite. I am not familiar with compiler-rt. Do you know how this problem occurs? Thank you very much if you can give me some advice！
llvm-jitlink error: Symbols not found: [ _ZTIi ]

These tests passed for me when I ran them on Debian Buster aarch64 a couple months ago. The _ZTIi symbol is the typeinfo for the int type; at least on my system this is available as a weak symbol in libstdc++. What distribution did you test? I'll rebuild and test the current main branch to verify that it still works for me.

I just ran llvm-lit and ran into this problem. I'm based on the main branch. I suspect that my running mode or running environment is faulty. Do these cases have any environment dependency? I'm currently based on the aarch64 platform

Which aarch64 Linux distribution are you using? How did you configure LLVM_ENABLE_PROJECTS and LLVM_ENABLE_RUNTIMES when you configured your build?

I updated to the tip of the main branch and re-built everything, and all supported ORC tests pass for me on Debian Buster using llvm-lit.

In D127720#3741484, @housel wrote:

In D127720#3741401, @TiehuZhang wrote:

Hi, @housel, the following error occurs when I run the test suite. I am not familiar with compiler-rt. Do you know how this problem occurs? Thank you very much if you can give me some advice！
llvm-jitlink error: Symbols not found: [ _ZTIi ]

These tests passed for me when I ran them on Debian Buster aarch64 a couple months ago. The _ZTIi symbol is the typeinfo for the int type; at least on my system this is available as a weak symbol in libstdc++. What distribution did you test? I'll rebuild and test the current main branch to verify that it still works for me.

Hi, @housel, the following error occurs when I run the test suite. I am not familiar with compiler-rt. Do you know how this problem occurs? Thank you very much if you can give me some advice！
llvm-jitlink error: Symbols not found: [ _ZTIi ]

Failed Tests (3):
  ORC-aarch64-linux :: TestCases/Linux/aarch64/ehframe-default.cpp
  ORC-aarch64-linux :: TestCases/Linux/aarch64/ehframe-libunwind.cpp
  ORC-aarch64-linux :: TestCases/Linux/aarch64/lljit-ehframe.cpp

In D118566#3605004, @dmgreen wrote:

Hello.

Hmm. I remember that this was important for performance in some cases on AArch64 - but we don't enable AggressivelyInterleaveReductions. If you have examples of scalar interleaving causing performance problems with AggressivelyInterleaveReductions then it sounds sensible to me to disable it for the if you pointed to as well.

Hi, @dmgreen, is it necessary to add ScalarInterleavingRequiresPredication to the guard ? I met some cases that the current patch wants to prevent from interleaving but still be interleaved, as AggressivelyInterleaveReductions is enabled on our target.

if (AggressivelyInterleaveReductions && !ScalarInterleavingRequiresPredication) {
  LLVM_DEBUG(dbgs() << "LV: Interleaving to expose ILP.\n");
  return IC;
}

In D124612#3559695, @dmgreen wrote:

Thanks. If you can update the test case, then this looks sensible to me.

In D124612#3536665, @dmgreen wrote:

Thanks for getting the numbers.

In D124612#3482219, @sdesmalen wrote:

Did you do any performance measurements to get an impression of the performance impact of this change?

In D124612#3486176, @dmgreen wrote:

Yes - do you have benchmarking results for this patch? This option makes sense, but I'm not sure what it's doing is always optimal. There's something going on with how it alters interleaving group costs, that doesn't look like it should be related to vector addresses. One such case was cleaned up (or maybe hidden) by D124786, but more problems might be present.

In D122126#3515070, @fhahn wrote:

Still LGTM, thanks! The remaining suggestion can be addressed directly before committing the patch.

The code has been updated since accept. Please review it again. Thank you very much! @fhahn @dmgreen

(Updated)
Difference with accepted version: Move memory runtime checks to processLoop to control both VF and IC

Fix the failed case (optimization-remark-options.c), because the remark info should be updated

In D122126#3413513, @dmgreen wrote:

Why does this check SelectedVF.Width > 1? Can we just remove it or does that not help?

Does this mean this didn't help? It seems that code would not alter the IC in any case, as it is already returning VectorizationFactor::Disabled.

Just as a point of cleanup, is it possible to move the logic into selectInterleaveCount, so that it returns a value of 1 if there are too many runtime checks? I'm not sure all the info needed would be easily available inside the costmodel though.

ping

In D111804#3065956, @craig.topper wrote:

In D111804#3065924, @TiehuZhang wrote:

In D111804#3065906, @craig.topper wrote:

If an input to an or instruction is poison, the output of the or is poison regardless of whether the other operand is true when the poison result occurs.

If an incorrect optimization is occurring it must be where the or is created.

Thanks for your review! @craig.topper. Do you mean that we should prevent this pattern from being generated, rather than remedying it after the pattern is generated? Actually, the issue I met started from D95959. That optimization does not seem to apply to all scenarios, such as those where the shift operand is negative. In the case I present, this optimization results in a miscalculation of the KnownBit, which in turn results in a false transformation.

The transform is correct based on the semantics of llvm's or instruction. The or instruction in llvm does not have short circuit evaluation. It can't block the spread of poison. The IR that has the semantics you want is

define i32 @cmp_lshr_cmp(i32 %0) {
entry:
  %cmp = icmp slt i32 %0, 0
  %shr = lshr i32 7, %0
  %cmp1 = icmp sgt i32 %0, %shr
  %or.cond = select i1 %cmp, i1 true, i1 %cmp1
  br i1 %or.cond, label %cond.end, label %cond.false

cond.false:                                       ; preds = %entry
  ret i32 1

cond.end:                                         ; preds = %entry, %cond.false
  ret i32 0
}

Using a select instruction instead of an or prevents the %cmp1 value from being used if %cmp is true. There was previously an incorrect transform that would turn such a select into an or, but it was disabled in https://reviews.llvm.org/D101191

In D111804#3065906, @craig.topper wrote:

If an input to an or instruction is poison, the output of the or is poison regardless of whether the other operand is true when the poison result occurs.

If an incorrect optimization is occurring it must be where the or is created.

In D111804#3064140, @lebedev.ri wrote:

Sorry, but i can not parse the description,

The current transformation is *not* a miscompile:
https://godbolt.org/z/WeTeasK53
https://alive2.llvm.org/ce/z/LVDqSX

Hi, @dmgreen, the previous implementation doesn't take the order of extends into account, so Instruction does not dominate all uses error will still appear in one of the testcases you mentioned. I have updated the patch to fix the failed case. Could you please check whether this modification is appropriate? Thanks very much.

In D107262#2936743, @dmgreen wrote:

I've committed 013030a0b213a75e0403fcdb5a070d21831ee561. Do you want to rebase and fix those extra testcases here? Or do you think that's best left for a separate patch?

In D107262#2930517, @dmgreen wrote:

Thanks, looking good. But I do still worry about the order of instructions sunk.

I was trying it out, seeing if it would go wrong when we were sinking a lot of operands. I noticed that the add/sub sinking wasn't really working properly though! There is https://reviews.llvm.org/D107623 to improve that and getting the shuffles to sink.

With that in, can you add these two test to show partially sinking two values at the same time:

define <4 x i32> @sinkadd_partial(<8 x i16> %a1, <8 x i16> %a2, i8 %f) {
for.cond4.preheader.lr.ph:
  %cmp = icmp slt i8 %f, 0
  %s2 = shufflevector <8 x i16> %a2, <8 x i16> poison, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  %s1 = shufflevector <8 x i16> %a1, <8 x i16> poison, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  br i1 %cmp, label %for.cond4.preheader.us.preheader, label %for.cond4.preheader.preheader

for.cond4.preheader.us.preheader:                 ; preds = %for.cond4.preheader.lr.ph
  %e1 = sext <4 x i16> %s1 to <4 x i32>
  %e2 = sext <4 x i16> %s2 to <4 x i32>
  %0 = add <4 x i32> %e1, %e2
  ret <4 x i32> %0

for.cond4.preheader.preheader:                    ; preds = %for.cond4.preheader.lr.ph
  ret <4 x i32> zeroinitializer
}

define <4 x i32> @sinkadd_partial_rev(<8 x i16> %a1, <8 x i16> %a2, i8 %f) {
for.cond4.preheader.lr.ph:
  %cmp = icmp slt i8 %f, 0
  %s2 = shufflevector <8 x i16> %a2, <8 x i16> poison, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  %s1 = shufflevector <8 x i16> %a1, <8 x i16> poison, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  br i1 %cmp, label %for.cond4.preheader.us.preheader, label %for.cond4.preheader.preheader

for.cond4.preheader.us.preheader:                 ; preds = %for.cond4.preheader.lr.ph
  %e2 = sext <4 x i16> %s2 to <4 x i32>
  %e1 = sext <4 x i16> %s1 to <4 x i32>
  %0 = add <4 x i32> %e1, %e2
  ret <4 x i32> %0

for.cond4.preheader.preheader:                    ; preds = %for.cond4.preheader.lr.ph
  ret <4 x i32> zeroinitializer
}

The order of extends in the target block become important.

In D107262#2922154, @dmgreen wrote:

In D107262#2921656, @TiehuZhang wrote:

In D107262#2919594, @dmgreen wrote:

This happens because out of the chain of insert; shuffle only the insert needs to be sunk?
It may be better to remove the UI->getParent() == TargetBB from if (UI->getParent() == TargetBB || isa<PHINode>(UI)) and update InsertPoint in the loop as we go. It won't need to clone the instruction, but it can update InsertPoint and continue.

Hi, dmgreen, thanks for your review! But I don't quite make sense of your comment. The condition UI->getParent() == TargetBB is used to filter out some instructions already in targetBB to avoid the meaningless sinking, why can it be removed?

shouldSinkOperands will return a chain of instructions, in this case starting at the mul it will return the shuffle and the insert, as those are the instruction it is profitable to sink. They are visited in reverse order in order to sink the last instruction first. The shuffle is already in the TargetBB, so doesn't need to be sunk (or cloned), but we still need to update the InsertPoint when sinking the insert, or else it will fail dominance checks.

It looks like there are some Arm tests failing with the current attempt to fix that. The instruction being sunk may have multiple uses, and moving before the first one we happen to find might not always work, it might not be the instruction that was originally returned by shouldSinkOperands.

Instead, I think it would be better to make sure we are updating InsertPoint as we go in the ToReplace loop. So we add instruction to ToReplace even if they are already in TargetDB, and in the ToReplace loop we check if the instruction is already in the correct BB and just update the InsertPoint if so.

In D107262#2919594, @dmgreen wrote:

This happens because out of the chain of insert; shuffle only the insert needs to be sunk?
It may be better to remove the UI->getParent() == TargetBB from if (UI->getParent() == TargetBB || isa<PHINode>(UI)) and update InsertPoint in the loop as we go. It won't need to clone the instruction, but it can update InsertPoint and continue.

ping

ping

ping

update the testcase