[PATCH] D113289: LICM: Hoist LOAD without STORE

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

Thanks a lot for your comments!

In D113289#3135476 <https://reviews.llvm.org/D113289#3135476>, @reames wrote:

> This needs much clearer motivation - specifically a clear explanation of what you're trying to achieve and why.  Please consider that motivation blocking for further review.

I've updated the summary as a first step.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:120-125
+// Hoist the load without hoisting the store. Some targets (such as aarch64)
+// will gain benefits out of this, since there will be better register
+// promotion.
+static cl::opt<bool> EnableLoadHoistOnly(
+    "licm-load-hoisting-only", cl::Hidden, cl::init(false),
+    cl::desc("Enable hoisting loads without hoisting the stores"));
----------------
lebedev.ri wrote:
> This is really ambiguous to me. Is this talking about
> *only* about hoisting loads, and preventing hoisting stores,
> or hoisting loads even if we can't hoist the store?
Oh, I agree...


================
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.
 ///
----------------
efriedma wrote:
> Maybe move this explanation to the definition of the variable "ShouldTryLoadHoistingOnly", since that's where it's relevant.
Makes sense.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2313
+      if (SavedInsts.count(I))
+        return false;
       return L->isLoopInvariant(SI->getPointerOperand());
----------------
efriedma wrote:
> 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.
Hmm, actually it doesn't improve anything, we should simply return false if there is no load to promote.


================
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
+
----------------
efriedma wrote:
> 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.
The more tests the better. I'll try to make some other tests as well. As a starting point, I'll update the CHECKS for this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113289