[PATCH] D90249: enable memoryssa for loopsink in new passmanager, expand loopsink testing and fix exposed bug in LICM

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

The current behavior is:

- for the NPM, always use MSSA for LoopSink, which is a Function pass.
- for the LPM, LoopSink is a Loop pass and it will use MSSA conditioned on the `EnableMSSALoopDependency` flag which is set to `true`, hence this patch is switching loop sink to use MSSA.

I would suggest adding a different cl::opt, specific to LoopSink and set it to false in this patch, hence not changing the current behavior. Use this flag for both pass managers (or create two flags if you intend to flip just one).
Then in a subsequent patch change the flag(s) to true and update the pipeline tests. 
This will provide a simple revert of the cl::opt value if things go wrong, and an option for potential users to disable the MSSA usage only for LoopSink, not for all the other passes (controlled by EnableMSSALoopDependency).



================
Comment at: llvm/lib/Transforms/Scalar/LoopSink.cpp:371
+                                             /*ScalarEvolution*/ nullptr,
+                                             nullptr, &MSSA);
   } while (!PreorderLoops.empty());
----------------
/*CurAST=*/nullptr


================
Comment at: llvm/lib/Transforms/Scalar/LoopSink.cpp:405
+    AAResults &AA = getAnalysis<AAResultsWrapperPass>().getAAResults();
+    AliasSetTracker CurAST(AA);
+
----------------
Only create AST in the absence of MSSA.
The checks in `canSinkOrHoistInst` assume only one of the two analyses is passed in. For LICM, the asserts for this are in sinkRegion/hoistRegion. I added a similar assert in f514b32a899 to ensure this is true here as well.


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

https://reviews.llvm.org/D90249