[opt][NewPM] Add basic-aa in legacy PM compatibility mode

[aeubanks]

Summary

The legacy PM alias analysis pipeline by default includes basic-aa.
When running opt -foo-pass under the NPM and -disable-basic-aa is not
specified, use basic-aa.

This decreases the number of check-llvm failures under NPM from 913 to 752.

[ychen]

This decreases the number of check-llvm failures under NPM from 913 to 752.

This is great!

llvm/tools/opt/NewPMDriver.cpp

351

I think it is helpful to be specific here, saying something like "AAResultsWrapperPass` by default provide basic-aa in legacy PM."

Could we combine this code with the if (!AAPipeline.empty()) code above? If the AAPipeline is empty, we could make it "basic-aa".

[asbirlea]

ltgm with ychen's comment addressed (use AAPipeline empty check and merge with the above check)

[aeubanks]

Add more detail in comments

llvm/tools/opt/NewPMDriver.cpp

351

Added comment.

I don't want to change the semantics of any tests using opt -passes=foo, this is purely for compatibility with tests using opt -foo-pass.

And actually I think having to explicitly specify basic-aa makes more sense than implicitly adding it, so I like the NPM way of doing it. But that's a different discussion.

[ychen]

llvm/tools/opt/NewPMDriver.cpp

351

That makes sense. Does it mean this code is temporary? i.e once we switch to NPM, we will remove this code and mass-adding basic-aa to relevant tests? If so, then sounds good to me.

[aeubanks]

llvm/tools/opt/NewPMDriver.cpp

351

I was thinking that would happen alongside whenever we migrate tests to use '-passes='.

[ychen]

llvm/tools/opt/NewPMDriver.cpp

351

Sounds good. Please add a comment about the next step related to this code.

Could you check the empty AAPipeline instead? The code right now is adding basic-aa to AAPipeline.

[ychen]

Please let @asbirlea acknowledge this also.

[aeubanks]

Add comment

llvm/tools/opt/NewPMDriver.cpp

351

Added comment.

AAPipeline has nothing to do with the legacy PM compatibility mode (there's an assert checking that it is empty if we are in the legacy PM compat mode). The code isn't adding basic-aa to AAPipeline, it's adding it to AA.

[ychen]

llvm/tools/opt/NewPMDriver.cpp

351

Oh, now I see the implicit assert above. I was looking for an assertion related to AAPipeline. Thanks. Looks good to me.

[asbirlea]

llvm/tools/opt/NewPMDriver.cpp

351

I honestly don't like the idea of doing this now just for the tests.

We've had out-of-tree testing in the community that relied on the -disable-aa flag, and on other particular command-line behavior for fuzz testing. This patch is changing the current NPM expected behavior.

We either change the behavior in this patch and stick to it, or we update the tests at this point and never change the behavior in the first place.
I think ping-ponging with expectations just to delay test changes is not a positive experience for such users.

[aeubanks]

llvm/tools/opt/NewPMDriver.cpp

351

Keeping the behavior in this patch going forward does sound better.
My main intention was that if we ever get rid of the opt -foo-pass syntax and migrate relevant tests to use opt -passes=foo -aa-pipeline=basic-aa, we can delete this. Not that we'd go back and add -basic-aa to opt -foo-pass tests later.

Maybe the comment I added isn't clear, but that's what I meant.

[asbirlea]

sgtm.

llvm/lib/Analysis/AliasAnalysis.cpp

 #include <cassert>
 #include <functional>
 #include <iterator>
 
 using namespace llvm;
 
 /// Allow disabling BasicAA from the AA results. This is particularly useful
 /// when testing to isolate a single AA implementation.
-static cl::opt<bool> DisableBasicAA("disable-basic-aa", cl::Hidden,
-                                    cl::init(false));
+cl::opt<bool> DisableBasicAA("disable-basic-aa", cl::Hidden, cl::init(false));
 
 AAResults::AAResults(AAResults &&Arg)
     : TLI(Arg.TLI), AAs(std::move(Arg.AAs)), AADeps(std::move(Arg.AADeps)) {
   for (auto &AA : AAs)
     AA->setAAResults(this);
 }
 
 AAResults::~AAResults() {

llvm/test/Transforms/SLPVectorizer/X86/limit.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -slp-vectorizer -S | FileCheck %s
+; RUN: opt < %s -slp-vectorizer -enable-new-pm -S | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @b = common global [4 x i32] zeroinitializer, align 16
 @c = common global [4 x i32] zeroinitializer, align 16
 @d = common global [4 x i32] zeroinitializer, align 16
 @e = common global [4 x i32] zeroinitializer, align 16

llvm/tools/opt/NewPMDriver.cpp

 
 // Individual pipeline tuning options.
 extern cl::opt<bool> DisableLoopUnrolling;
 
 extern cl::opt<PGOKind> PGOKindFlag;
 extern cl::opt<std::string> ProfileFile;
 extern cl::opt<CSPGOKind> CSPGOKindFlag;
 extern cl::opt<std::string> CSProfileGenFile;
+extern cl::opt<bool> DisableBasicAA;
 
 static cl::opt<std::string>
     ProfileRemappingFile("profile-remapping-file",
                          cl::desc("Path to the profile remapping file."),
                          cl::Hidden);
 static cl::opt<bool> DebugInfoForProfiling(
     "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
     cl::desc("Emit special debug info to enable PGO profile generation."));
if (PB.isAAPassName(PassName)) {
       if (auto Err = PB.parseAAPipeline(AA, PassName)) {
         errs() << Arg0 << ": " << toString(std::move(Err)) << "\n";
         return false;
       }
     } else {
       NonAAPasses.push_back(PassName);
     }
   }
+  // For compatibility with the legacy PM AA pipeline.
+  // AAResultsWrapperPass by default provides basic-aa in the legacy PM
+  // unless -disable-basic-aa is specified.
+  // TODO: remove this once tests implicitly requiring basic-aa use -passes= and
+  // -aa-pipeline=basic-aa.
+  if (!Passes.empty() && !DisableBasicAA) {
+    if (auto Err = PB.parseAAPipeline(AA, "basic-aa")) {
+      errs() << Arg0 << ": " << toString(std::move(Err)) << "\n";
+      return false;
+    }
+  }
 
   LoopAnalysisManager LAM(DebugPM);
   FunctionAnalysisManager FAM(DebugPM);
   CGSCCAnalysisManager CGAM(DebugPM);
   ModuleAnalysisManager MAM(DebugPM);
 
   // Register the AA manager first so that our version is the one used.
   FAM.registerPass([&] { return std::move(AA); });