[PATCH] D113289: LICM: Hoist LOAD without STORE

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2012
+/// preserving the stores as is, since it may improve register promotion
+/// for some targets. This is controled by the EnableLoadHoistOnly flag.
 ///
----------------
Maybe move this explanation to the definition of the variable "ShouldTryLoadHoistingOnly", since that's where it's relevant.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2313
+      if (SavedInsts.count(I))
+        return false;
       return L->isLoopInvariant(SI->getPointerOperand());
----------------
Not sure I understand what you're doing with SavedInsts here.

If I'm following correctly, SavedInsts is empty the first time this is called from LoopInvariantCodeMotion::runOnLoop.  On subsequent iterations, it contains the stores that correspond to loads that were optimized.

If you're trying to prevent an infinite loop, this isn't the way to do it; promoteLoopAccessesToScalars should just return false if there aren't any loads to promote.  If you're just trying to save a little work in subsequent iterations, please leave it out for now; if there's actually some significant compile-time gain here, we can look into it as a followup.


================
Comment at: llvm/test/Transforms/LICM/AArch64/hoist-load-without-store.ll:20
+; CHECK: %u.promoted = load i32, i32* @u
+; CHECK: %v.promoted = load i32, i32* @v
+
----------------
I'd like to see check lines for the PHI nodes.

Do we want any other tests here?  Doesn't feel right to have just one test.  I guess the cases where this transforms fails are generally covered by other tests, though, so not sure what we'd need to cover.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113289