[PATCH] D50426: [NFC][MustExecute] Rework API to start making better analysis

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: include/llvm/Analysis/MustExecute.h:44
 
-  LoopSafetyInfo() = default;
-};
+  LoopSafetyInfo(Loop *L, DominatorTree *DT) {
+    // TODO: DT will later be used by ICF analysis that will come in a follow-up
----------------
reames wrote:
> Drop the DT from this patch.  That wasn't the API change with the risky implications.
> 
> Also, shouldn't L always be non-null?  If so, assert or use reference.  
> 
> Reading down in the patch, I see that L can be null.  You'd be better with two constructors here.  One which explicitly created an uninitialized object and the other which always created an initialized one.
If we check it on non-null, it fails some unit tests. I can add add this assertion only with unit tests modification in a separate patch.


================
Comment at: include/llvm/Analysis/MustExecute.h:53
+
+  bool mayThrow(const BasicBlock *BB);
+
----------------
fedor.sergeev wrote:
> reames wrote:
> > Should be const functions.
> This most definitely is a "preparation" for D50377, where LoopSafetyInfo gets more state and mayThrow becomes non-const.
> 
> Said that, I do question the need for doing a separate API change that goes w/o a semantical context that necessitates the change.
> 
I agree with Fedor, I would rather not do all these methods `const` because in the follow-up they all become non-const because of caching added.


================
Comment at: lib/Analysis/MustExecute.cpp:120
                                  const DominatorTree *DT, const Loop *CurLoop,
-                                 const LoopSafetyInfo *SafetyInfo) {
+                                 LoopSafetyInfo *SafetyInfo) {
   // We have to check to make sure that the instruction dominates all
----------------
reames wrote:
> Why drop the const?
Because in the follow-up patch for which this API change was made https://reviews.llvm.org/D50377, caching is added and this becomes non-const. I can leave const here, but then it will be dropped in this patch. The entire idea of making API change separately is to avoid modifying a lot of files with changes like that.

Please do not consider this patch as being something self-valuable, it will only be merged with follow-up. That is why all constant modifiers changes are made here. It's for convenience of the reviewers of functional change, not because it is something needed in NFC.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:97
 static bool hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
-                  const LoopSafetyInfo *SafetyInfo,
                   OptimizationRemarkEmitter *ORE);
----------------
reames wrote:
> If you need to drop const everywhere, you need to revisit your API.  (clue: you don't)
Adding caching logic always leads to drop of const modifiers. We can leave it const and super-slow though. :)


https://reviews.llvm.org/D50426