[PATCH] D50501: [LICM] Hoist guards with invariant conditions

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/mandatory separate cleanup changes as described.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:523
+          CurLoop->hasLoopInvariantOperands(&I)) {
+        bool Hoisted = hoist(I, DT, CurLoop, SafetyInfo, ORE);
+        Changed |= Hoisted;
----------------
mkazantsev wrote:
> reames wrote:
> > hoist always returns true.  Separately, change it to return void and update other caller.  
> Not sure if it's a good idea... What if it changes in the future? Besides, `sink` can actually return `false`, and it would be strange to have different signatures for these two.
This was not a suggestion.  Please make the change.  It can be done post commit, but I want this cleanup done.  At the moment, you have control flow in this patch which is never executed and introduces non-trivial reasoning complexity for no value.  

In general, the person who changes code in the future has to think about the implications of their changes.  Future proofing never works and just adds complexity now.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:523
+      using namespace PatternMatch;
+      if (match(&I, m_Intrinsic<Intrinsic::experimental_guard>()) &&
+          IsMustExecute && IsMemoryNotModified &&
----------------
Note for anyone reading:  This can be trivially extended to llvm.invariant.start w/no uses.  I thought about requiring that in this review, but decided a separate patch was better.  


https://reviews.llvm.org/D50501