[PATCH] D137707: Move "auto-init" instructions to the dominator of their users

Mon Dec 5 15:15:01 PST 2022

fhahn added inline comments.

================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:888
   EarlyFPM.addPass(EarlyCSEPass());
+  EarlyFPM.addPass(MoveAutoInitPass());
   if (Level == OptimizationLevel::O3)
----------------
What's the rational behind placing it here and not closer to other memory optimizations like DSE/MemCpyOpt?

================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:93
+    // The remaining logic should be pretty similar.
+    if (!hasAutoInitMetadata(I))
+      continue;
----------------
efriedma wrote:
> Can we avoid iterating over the function if it doesn't have any auto-init?
It's not clear to me from the FIXME why we would want to limit this to `auto-init` instructions. Shouldn't this be beneficial for all memory instructions?

================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:182
+  PreservedAnalyses PA;
+  PA.preserve<DominatorTreeAnalysis>();
+  return PA;
----------------
This pass moves only memory instructions, so could it be marked as preserving all CFG analyses?

This should ideally also preserve MemorySSA.

================
Comment at: llvm/test/Transforms/MoveAutoInit/branch.ll:5
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
does this depend on the triple? If it does, this needs `REQUIRES:...` otherwise it will fail if the X86 backend is not built. (same for all tests)

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

https://reviews.llvm.org/D137707