[PATCH] D94308: [MachineSink] SinkIntoLoop: analyse stores and aliases in between

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:380
 
+    // This e.g. skips branche and store instructions.
     const MachineOperand &MO = MI.getOperand(0);
----------------
branches. (Store instructions can still have defs, as in for post-inc's)


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:387
-    bool DontMoveAcrossStore = true;
-    if (!MI.isSafeToMove(AA, DontMoveAcrossStore)) {
-      LLVM_DEBUG(dbgs() << "LoopSink: Instruction not safe to move.\n");
----------------
Why remove this call to isSafeToMove? Don't we need to check for stores/exceptions/unmodelled side effects/etc?


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1229
+  // 3) Check all instruction in the sink block, to see if they alias.
+  for (auto &CurI : *SinkTo) {
+    if (AreAliased(CurI, I, I.getParent(), SinkTo, {}, SawStore, HasAliasedStore)) {
----------------
If it's sinking into a loop, does it not have to check all the instructions in the loop for aliasing?


================
Comment at: llvm/test/CodeGen/AArch64/loop-sink.mir:969-970
   ; CHECK:   [[PHI2:%[0-9]+]]:gpr32 = PHI [[COPY]], %bb.1, %4, %bb.3
+  ; CHECK:   [[ADRP:%[0-9]+]]:gpr64common = ADRP target-flags(aarch64-page) @A
+  ; CHECK:   [[LDRWui:%[0-9]+]]:gpr32 = LDRWui killed [[ADRP]], target-flags(aarch64-pageoff, aarch64-nc) @A :: (dereferenceable load 4 from `i32* getelementptr inbounds ([100 x i32], [100 x i32]* @A, i64 0, i64 0)`)
   ; CHECK:   [[SDIVWr:%[0-9]+]]:gpr32 = SDIVWr [[PHI2]], [[LDRWui]]
----------------
Some of these look very expensive to avoid a stack reload. Is the reload not cheaper, even if it is in the loop?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94308/new/

https://reviews.llvm.org/D94308