[PATCH] D25873: [LCSSA] Perform LCSSA verification only for current loop nest

[Michael Zolotukhin via llvm-commits]

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

Hi Igor,

Sorry for the delay in review. We've discussed this issue already, and the patch does seem like what we agreed on (modulo minor nitpicks).

However, I still feel uneasy about a couple of things. First, adding an extra fake pass for LCSSA verification looks really awkward (I have read the explanation you provided). Second, this all is caused by the fact that not all passes currently require LCSSA, but I'd rather change that, even independently on the patch in question. Do we have other loop passes, except the printer pass, that don't require LCSSA?

Also, related, but not affecting the patch question: what are the slowdowns that you see with full verification after you fixed the recursive verification routine?

I think that this patch can be committed so that your work is no longer affected by the slowdowns, but I would like to continue this discussion and consider other solutions again.

Thanks,
Michael



================
Comment at: include/llvm/Analysis/LoopPass.h:160
+// LPPassManager to check if current pass preserves lcssa form, and if it does
+// pass manager calls lcssa verification for the current loop.
+struct LCSSAVerificationPass : public FunctionPass {
----------------
s/lcssa/LCSSA/


================
Comment at: include/llvm/InitializePasses.h:171
+void initializeLCSSAVerificationPassPass(PassRegistry &);
+void initializeLegacyLICMPassPass(PassRegistry &);
 void initializeLazyBranchProbabilityInfoPassPass(PassRegistry&);
----------------
This change looks unnecessary (there are no spaces before `&` in lines below anyway).


================
Comment at: lib/Transforms/Utils/LCSSA.cpp:336
+    // up to 10x slowdown. Currently it's disabled by default. LPPassManager
+    // always does limited form of the lcssa verification. Similar reasoning
+    // was used for the LoopInfo verifier.
----------------
s/lcssa/LCSSA/
Let's be consistent :-)



https://reviews.llvm.org/D25873