[PATCH] D50377: [MustExecute] Rework LoopSafetyInfo to make it more optimistic about throws

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Another round of bugs found.

I am generally concerned about the lack of progress towards a clearly correct patch here.  I think we need to revisit how this is being approached and find a way to split this into individually obviously correct pieces.

One possible split (not the only one) would be the following:

1. Add ICF info to LoopSafetyInfo, but don't remove HeaderMayThrow approach.  Enable the use of ICF (for asserts only) under an off by default flag.  (Flag could be a param to constructor or cl::opt?)
2. For each pass, review in isolation.  Update tests to use off by default flag.
3. Once *all* passes are done, change default and remove flag.  (But still only ICF for asserts!)
4. Wait 1 week.
5. Remove HeaderMayThrow, and enhance logic.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1267
 
   Value *SomePtr = *PointerMustAliases.begin();
   BasicBlock *Preheader = CurLoop->getLoopPreheader();
----------------
reames wrote:
> There needs to be some invalidations somewhere within this function.
It doesn't look like my previous comment has been addressed.  There's still a bug here.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:187
     bool SanitizeMemory;
-    LoopSafetyInfo SafetyInfo;
+    LoopSafetyInfo *SafetyInfo;
 
----------------
Default init to nullptr.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:1558
       if (LI->replacementPreservesLCSSAForm(I, V)) {
+        SafetyInfo->dropCachedInfo(I->getParent());
         ReplaceUsesOfWith(I, V, Worklist, L, LPM);
----------------
Why?  I think this is unnecessary.


https://reviews.llvm.org/D50377