Ensure all uses of permute instruction feed vector stores

[kbarton]

Summary

There is a problem in VSXSwapRemoval where it is incorrectly removing permute instructions.
In this case, the permute is feeding both a vector store and also a non-store instruction. In this case, the permute cannot be removed.

The fix is to simply look at all the uses of the vector register defined by the permute and ensure that all the uses are vector store instructions.

This problem was reported in PR 27735 (https://llvm.org/bugs/show_bug.cgi?id=27735).

Test case based on the original problem reported.

[nemanjai]

I'm just curious how this happens since the swap instruction that defines the register for the store is emitted together with the actual store. How is there another user for a node that the back end added as part of DAG combine? I assume this is some sort of CSE that is running between the combine step and the swap removal step? If it is, might it also be advantageous to teach it not to eliminate expressions when one of them is a swap feeding a store?

I'm not suggesting this is necessarily the right thing to do, but one thing that will prevent MachineCSE from eliminating swaps is by adding let hasSideEffects = 1 in the definition of XXPERMDI. That fixes this issue, but has the disadvantage of disabling CSE for other forms of the instruction.

In any case, disabling MachineCSE for swaps might be something we want to consider.

[wschmidt]

The patch LGTM. One question: Did you check for the reverse situation? Can we have a swap that is fed by more than one instruction, where one is not a load, or is that already handled in the code? I realize this may not come up in practice if we don't have a machine sinking pass.

@nemanjai, yes, you could disable other optimizations, but I wouldn't recommend it. If one were going to spend more time on swap optimization, I'd instead recommend optimizing the web but inserting swaps in specific cases like this where something other than an STXVD2X occurs, which would always be a win in practice (though you could construct pathological cases where the code could get larger, though still no slower). But since P8 optimization is off the table right now, I'd just leave things lie.

[nemanjai]

In D21802#469887, @wschmidt wrote:

The patch LGTM. One question: Did you check for the reverse situation? Can we have a swap that is fed by more than one instruction, where one is not a load, or is that already handled in the code? I realize this may not come up in practice if we don't have a machine sinking pass.

@nemanjai, yes, you could disable other optimizations, but I wouldn't recommend it. If one were going to spend more time on swap optimization, I'd instead recommend optimizing the web but inserting swaps in specific cases like this where something other than an STXVD2X occurs, which would always be a win in practice (though you could construct pathological cases where the code could get larger, though still no slower). But since P8 optimization is off the table right now, I'd just leave things lie.

Excellent, thanks for answering the question Bill. I really only asked it because I don't know enough about this to know if allowing MachineCSE to eliminate swaps might lead to other situations where we do the wrong thing in our pass.

[kbarton]

In D21802#469731, @nemanjai wrote:

I'm just curious how this happens since the swap instruction that defines the register for the store is emitted together with the actual store. How is there another user for a node that the back end added as part of DAG combine? I assume this is some sort of CSE that is running between the combine step and the swap removal step? If it is, might it also be advantageous to teach it not to eliminate expressions when one of them is a swap feeding a store?

I'm not suggesting this is necessarily the right thing to do, but one thing that will prevent MachineCSE from eliminating swaps is by adding let hasSideEffects = 1 in the definition of XXPERMDI. That fixes this issue, but has the disadvantage of disabling CSE for other forms of the instruction.

In any case, disabling MachineCSE for swaps might be something we want to consider.

I thought about disabling MachineCSE in this case, but after talking to Bill I decided it wasn't the right approach. If we do disable it, we would need to do it through the cost model. There isn't a flag that we can use to make MachineCSE illegal on swap instructions that wouldn't have broader impacts throughout the optimizer. Marking XXPERMDI as having side effects, for example, can limit many other things that move code around, which we don't want.

There is also the problem that XXPERMDI doesn't really have side effects, so marking it to disable MachineCSE would likely draw some "attention" during reviews ;)

[kbarton]

In D21802#469887, @wschmidt wrote:

The patch LGTM. One question: Did you check for the reverse situation? Can we have a swap that is fed by more than one instruction, where one is not a load, or is that already handled in the code? I realize this may not come up in practice if we don't have a machine sinking pass.

This is already handled in the code. If the register defined by a swapping load is used in something other than a swapping load/swapping store, then the web is rejected. Unless there is another case outside of this that I'm not seeing....

[hfinkel]

In D21802#470162, @kbarton wrote:

In D21802#469731, @nemanjai wrote:

I'm just curious how this happens since the swap instruction that defines the register for the store is emitted together with the actual store. How is there another user for a node that the back end added as part of DAG combine? I assume this is some sort of CSE that is running between the combine step and the swap removal step? If it is, might it also be advantageous to teach it not to eliminate expressions when one of them is a swap feeding a store?

I'm not suggesting this is necessarily the right thing to do, but one thing that will prevent MachineCSE from eliminating swaps is by adding let hasSideEffects = 1 in the definition of XXPERMDI. That fixes this issue, but has the disadvantage of disabling CSE for other forms of the instruction.

In any case, disabling MachineCSE for swaps might be something we want to consider.

I thought about disabling MachineCSE in this case, but after talking to Bill I decided it wasn't the right approach. If we do disable it, we would need to do it through the cost model. There isn't a flag that we can use to make MachineCSE illegal on swap instructions that wouldn't have broader impacts throughout the optimizer. Marking XXPERMDI as having side effects, for example, can limit many other things that move code around, which we don't want.

There is also the problem that XXPERMDI doesn't really have side effects, so marking it to disable MachineCSE would likely draw some "attention" during reviews ;)

Indeed it would. ;) -- There are certainly cases where we don't want to CSE certain kinds of instructions (generally because of macro fusion and other microarchitectural pattern matching, to manage register pressure, etc.), but in those cases, you'd want to remateralize later instead of preventing CSE earlier (because CSEing the swap might enable CSEing other things).

[wschmidt]

In D21802#470163, @kbarton wrote:

In D21802#469887, @wschmidt wrote:

The patch LGTM. One question: Did you check for the reverse situation? Can we have a swap that is fed by more than one instruction, where one is not a load, or is that already handled in the code? I realize this may not come up in practice if we don't have a machine sinking pass.

This is already handled in the code. If the register defined by a swapping load is used in something other than a swapping load/swapping store, then the web is rejected. Unless there is another case outside of this that I'm not seeing....

Great, thanks. That was my only concern.

[kbarton]

Committed with r274645 = b4b959a20eac7f5cab1a86c17a85063627a66615 (refs/remotes/origin/master)

lib/Target/PowerPC/PPCVSXSwapRemoval.cpp

else if (SwapVector[EntryIdx].IsLoad && SwapVector[EntryIdx].IsSwap) {
       }
 
     // Reject webs that contain swapping stores that are fed by something
     // other than a swap instruction.
     } else if (SwapVector[EntryIdx].IsStore && SwapVector[EntryIdx].IsSwap) {
       MachineInstr *MI = SwapVector[EntryIdx].VSEMI;
       unsigned UseReg = MI->getOperand(0).getReg();
       MachineInstr *DefMI = MRI->getVRegDef(UseReg);
+      unsigned DefReg = DefMI->getOperand(0).getReg();
       int DefIdx = SwapMap[DefMI];
 
       if (!SwapVector[DefIdx].IsSwap || SwapVector[DefIdx].IsLoad ||
           SwapVector[DefIdx].IsStore) {
 
         SwapVector[Repr].WebRejected = 1;
 
         DEBUG(dbgs() <<
               format("Web %d rejected for store not fed by swap\n", Repr));
         DEBUG(dbgs() << "  def " << DefIdx << ": ");
         DEBUG(DefMI->dump());
         DEBUG(dbgs() << "  use " << EntryIdx << ": ");
         DEBUG(MI->dump());
         DEBUG(dbgs() << "\n");
       }
+
+      // Ensure all uses of the register defined by DefMI feed store
+      // instructions
+      for (MachineInstr &UseMI : MRI->use_nodbg_instructions(DefReg)) {
+        int UseIdx = SwapMap[&UseMI];
+
+        if (SwapVector[UseIdx].VSEMI->getOpcode() != MI->getOpcode()) {
+          SwapVector[Repr].WebRejected = 1;
+
+          DEBUG(dbgs() <<
+                format("Web %d rejected for swap not feeding only stores\n",
+                       Repr));
+          DEBUG(dbgs() << "  def " << " : ");
+          DEBUG(DefMI->dump());
+          DEBUG(dbgs() << "  use " << UseIdx << ": ");
+          DEBUG(SwapVector[UseIdx].VSEMI->dump());
+          DEBUG(dbgs() << "\n");
+        }
+      }
     }
   }
 
   DEBUG(dbgs() << "Swap vector after web analysis:\n\n");
   dumpSwapVector();
 }
 
 // Walk the swap vector entries looking for swaps fed by permuting loads

test/CodeGen/PowerPC/swaps-le-7.ll

+; RUN: llc -mcpu=pwr8 -mtriple=powerpc64le-unknown-linux-gnu -O3 < %s | FileCheck %s
+;
+; This is a regression test based on https://llvm.org/bugs/show_bug.cgi?id=27735
+;
+
+@G1 = global <2 x double> <double 2.0, double -10.0>
+@G2 = global <2 x double> <double 3.0, double 4.0>
+@G3 = global <2 x double> <double 5.0, double 6.0>
+@G4 = global <2 x double> <double 7.0, double 8.0>
+
+; CHECK-LABEL: @zg
+; CHECK: xxspltd
+; CHECK-NEXT: xxspltd
+; CHECK-NEXT: xxswapd
+; CHECK-NEXT: xvmuldp
+; CHECK-NEXT: xvmuldp
+; CHECK-NEXT: xvsubdp
+; CHECK-NEXT: xvadddp
+; CHECK-NEXT: xxpermdi
+; CHECK-NEXT: xvsubdp
+; CHECK-NEXT: xxswapd
+; CHECK-NEXT: stxvd2x
+; CHECK-NEXT: blr
+
+; Function Attrs: noinline
+define void @zg(i8* %.G0011_640.0, i8* %.G0012_642.0, <2 x double>* %JJ, <2 x double>* %.ka0000_391, double %.unpack, double %.unpack66) #0 {
+L.JA291:
+  %Z.L.JA291.2 = load <2 x double>, <2 x double>* %.ka0000_391, align 16
+  store <2 x double> %Z.L.JA291.2, <2 x double>* %JJ, align 8
+  %Z.L.JA291.3 = bitcast i8* %.G0012_642.0 to <2 x double>*
+  %Z.L.JA291.4 = load <2 x double>, <2 x double>* %Z.L.JA291.3, align 1
+  %.elt136 = bitcast i8* %.G0011_640.0 to double*
+  %.unpack137 = load double, double* %.elt136, align 1
+  %.elt138 = getelementptr inbounds i8, i8* %.G0011_640.0, i64 8
+  %Z.L.JA291.5 = bitcast i8* %.elt138 to double*
+  %.unpack139 = load double, double* %Z.L.JA291.5, align 1
+  %Z.L.JA291.6 = insertelement <2 x double> undef, double %.unpack137, i32 0
+  %Z.L.JA291.7 = insertelement <2 x double> %Z.L.JA291.6, double %.unpack137, i32 1
+  %Z.L.JA291.8 = fmul <2 x double> %Z.L.JA291.2, %Z.L.JA291.7
+  %Z.L.JA291.9 = shufflevector <2 x double> %Z.L.JA291.2, <2 x double> undef, <2 x i32> <i32 1, i32 0>
+  %Z.L.JA291.10 = insertelement <2 x double> undef, double %.unpack139, i32 0
+  %Z.L.JA291.11 = insertelement <2 x double> %Z.L.JA291.10, double %.unpack139, i32 1
+  %Z.L.JA291.12 = fmul <2 x double> %Z.L.JA291.9, %Z.L.JA291.11
+  %Z.L.JA291.13 = fsub <2 x double> %Z.L.JA291.8, %Z.L.JA291.12
+  %Z.L.JA291.14 = fadd <2 x double> %Z.L.JA291.8, %Z.L.JA291.12
+  %Z.L.JA291.15 = shufflevector <2 x double> %Z.L.JA291.13, <2 x double> %Z.L.JA291.14, <2 x i32> <i32 0, i32 3>
+  %Z.L.JA291.16 = fsub <2 x double> %Z.L.JA291.4, %Z.L.JA291.15
+  %Z.L.JA291.17 = bitcast i8* %.G0012_642.0 to <2 x double>*
+  store <2 x double> %Z.L.JA291.16, <2 x double>* %Z.L.JA291.17, align 8
+  %.. = bitcast <2 x double>* %JJ to i32*
+  %.pre = load i32, i32* %.., align 32
+  ret void
+}
+
+attributes #0 = { noinline }
 No newline at end of file