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

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:888
   EarlyFPM.addPass(EarlyCSEPass());
+  EarlyFPM.addPass(MoveAutoInitPass());
   if (Level == OptimizationLevel::O3)
----------------
fhahn wrote:
> fhahn wrote:
> > What's the rational behind placing it here and not closer to other memory optimizations like DSE/MemCpyOpt?
> (moving it closer to those will also allow re-using existing memorySSA)
+1 for that. The previous implementation was only using Dominator trees so it made sense to activate it early in the pipeline. This version is sensibly more costly, so it will propably appear at higher optimization level.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:93
+    // The remaining logic should be pretty similar.
+    if (!hasAutoInitMetadata(I))
+      continue;
----------------
fhahn wrote:
> 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?
> Can we avoid iterating over the function if it doesn't have any auto-init?

I could try a module pass and walk through the users of the auto-init metadata?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:93
+    // The remaining logic should be pretty similar.
+    if (!hasAutoInitMetadata(I))
+      continue;
----------------
serge-sans-paille wrote:
> fhahn wrote:
> > 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?
> > Can we avoid iterating over the function if it doesn't have any auto-init?
> 
> I could try a module pass and walk through the users of the auto-init metadata?
The algorithm beyond auto-init requires more care (it probably needs a reverse transversal of the CFG, and some optimization to avoid rebuilding the dominator of the users for each instruction). I'd rather start small with that pass and then upgrade it to a full-fledge version.


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

https://reviews.llvm.org/D137707