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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer 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);
----------------
dmgreen wrote:
> branches. (Store instructions can still have defs, as in for post-inc's)
Will clarify this comment a bit.


================
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");
----------------
dmgreen wrote:
> Why remove this call to isSafeToMove? Don't we need to check for stores/exceptions/unmodelled side effects/etc?
This has not been deleted but, it has been moved to line 1290 and is now part of `MachineSinking::IsSafeToMove()`. The reason is that we need to have found the sink block and loop in order analyse `DontMoveAcrossStore`.


================
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)) {
----------------
dmgreen wrote:
> If it's sinking into a loop, does it not have to check all the instructions in the loop for aliasing?
Yeah, good point! I was assuming/using single block loops, but not all loops confirm to that standard. ;-)
This should indeed check all loop blocks/instructions.


================
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]]
----------------
dmgreen wrote:
> Some of these look very expensive to avoid a stack reload. Is the reload not cheaper, even if it is in the loop?
Yep, that might very well be the case. But this is about building up the framework to support loop sinking which requires quite a bit of surgery: after this analysis/correctness patch, we indeed need to work on the cost-model/profitability (your remark), we may need options to force or limit the number of sinks, and then we need to enable this by default. Thus, quite a few steps to go, and cost analysis will be next after this patch.


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

https://reviews.llvm.org/D94308