[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Valentin Churavy via Phabricator via cfe-commits]

vchuravy added a comment.

> We'd need a lot more testing before we can claim to support reusing pipelines.

Sure, but moving pass pipeline state into the context makes it much harder to 
do this kind of testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D137149#4653381 , @vchuravy wrote:

> In D137149#4653308 , @aeubanks 
> wrote:
>
>> we don't currently support reusing a pipeline so I'm surprised that you're 
>> able to share/reuse pipelines without running into any issues
>
> In addition to @pchintalapudi's comment. Reuse of pipelines is something that 
> we (JuliaLang) had come to expect from old PM. This is why opt had a 
> `-run-twice` option to help flush
> out bugs that arose out of the idea that passes would only be used once. 
> @loladiro might remember those discussions.
>
> So when we ported Julia to NewPM I didn't think twice and @pchintalapudi 
> implemented our NewPM usage such that only the AnalysisManager
> would be created fresh.

Passes can store state that might break between runs (especially module passes 
since they typically only run once). I'm not saying it's impossible to reuse a 
pipeline, just that it's not tested, e.g. `-run-twice` isn't hooked up to the 
new PM. There was a discussion about this before somewhere, can't remember 
where. We'd need a lot more testing before we can claim to support reusing 
pipelines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Valentin Churavy via Phabricator via cfe-commits]

vchuravy added a subscriber: loladiro.
vchuravy added a comment.

In D137149#4653308 , @aeubanks wrote:

> we don't currently support reusing a pipeline so I'm surprised that you're 
> able to share/reuse pipelines without running into any issues

In addition to @pchintalapudi's comment. Reuse of pipelines is something that 
we (JuliaLang) had come to expect from old PM. This is why opt had a 
`-run-twice` option to help flush
out bugs that arose out of the idea that passes would only be used once. 
@loladiro might remember those discussions.

So when we ported Julia to NewPM I didn't think twice and @pchintalapudi 
implemented our NewPM usage such that only the AnalysisManager
would be created fresh.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Prem Chintalapudi via Phabricator via cfe-commits]

pchintalapudi added a comment.

In D137149#4653308 , @aeubanks wrote:

> we don't currently support reusing a pipeline so I'm surprised that you're 
> able to share/reuse pipelines without running into any issues

The way I see it, any pipeline state that can't be reused between runs is 
encapsulated within either the module or the analysis managers, with the 
analysis managers containing derived state that's expensive to compute about 
the module. Under that interpretation, the optimization passes are essentially 
stateless, so there's no reason they can't be run on multiple IR units (and 
CGSCC/Function/Loop passes must deal with this fact already since there's 
possibly more than one present per module). I don't know how much penalty there 
is for requiring passes to clean up after themselves, but I can't imagine that 
it's a very high cost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

we don't currently support reusing a pipeline so I'm surprised that you're able 
to share/reuse pipelines without running into any issues


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Valentin Churavy via Phabricator via cfe-commits]

vchuravy added a comment.
Herald added a subscriber: wangpc.

Late to the party here, but passing an `LLVMContext` seems really strange from 
a JIT perspective.
We construct a pipeline that may be run over many modules each of them maybe 
using a different LLVM Context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG721f975d3518: Use PassGate from LLVMContext if any otherwise 
global one (authored by Evgeniy Brevnov ybrev...@azul.com).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  flang/lib/Frontend/FrontendActions.cpp
  llvm/include/llvm/IR/OptBisect.h
  llvm/include/llvm/Passes/StandardInstrumentations.h
  llvm/lib/Analysis/CallGraphSCCPass.cpp
  llvm/lib/Analysis/LoopPass.cpp
  llvm/lib/Analysis/RegionPass.cpp
  llvm/lib/IR/LLVMContextImpl.cpp
  llvm/lib/IR/OptBisect.cpp
  llvm/lib/IR/Pass.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Passes/PassBuilderBindings.cpp
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/IR/LegacyPassManagerTest.cpp
  llvm/unittests/IR/PassManagerTest.cpp

Index: llvm/unittests/IR/PassManagerTest.cpp
===
--- llvm/unittests/IR/PassManagerTest.cpp
+++ llvm/unittests/IR/PassManagerTest.cpp
@@ -826,7 +826,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
@@ -872,7 +872,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
@@ -937,7 +937,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
Index: llvm/unittests/IR/LegacyPassManagerTest.cpp
===
--- llvm/unittests/IR/LegacyPassManagerTest.cpp
+++ llvm/unittests/IR/LegacyPassManagerTest.cpp
@@ -359,10 +359,8 @@
 struct CustomOptPassGate : public OptPassGate {
   bool Skip;
   CustomOptPassGate(bool Skip) : Skip(Skip) { }
-  bool shouldRunPass(const Pass *P, StringRef IRDescription) override {
-if (P->getPassKind() == PT_Module)
-  return !Skip;
-return OptPassGate::shouldRunPass(P, IRDescription);
+  bool shouldRunPass(const StringRef PassName, StringRef IRDescription) override {
+return !Skip;
   }
   bool isEnabled() const override { return true; }
 };
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -354,8 +354,8 @@
   PrintPassOptions PrintPassOpts;
   PrintPassOpts.Verbose = DebugPM == DebugLogging::Verbose;
   PrintPassOpts.SkipAnalyses = DebugPM == DebugLogging::Quiet;
-  StandardInstrumentations SI(DebugPM != DebugLogging::None, VerifyEachPass,
-  PrintPassOpts);
+  StandardInstrumentations SI(M.getContext(), DebugPM != DebugLogging::None,
+  VerifyEachPass, PrintPassOpts);
   SI.registerCallbacks(PIC, );
   DebugifyEachInstrumentation Debugify;
   DebugifyStatsMap DIStatsMap;
Index: llvm/lib/Passes/StandardInstrumentations.cpp
===
--- llvm/lib/Passes/StandardInstrumentations.cpp
+++ llvm/lib/Passes/StandardInstrumentations.cpp
@@ -767,27 +767,35 @@
   return ShouldRun;
 }
 
-void OptBisectInstrumentation::registerCallbacks(
+bool OptPassGateInstrumentation::shouldRun(StringRef PassName, Any IR) {
+  if (isIgnored(PassName))
+return true;
+
+  bool ShouldRun =
+  Context.getOptPassGate().shouldRunPass(PassName, getIRName(IR));
+  if (!ShouldRun && !this->HasWrittenIR && !OptBisectPrintIRPath.empty()) {
+// FIXME: print IR if limit is higher than number of opt-bisect
+// invocations
+this->HasWrittenIR = true;
+const Module *M = unwrapModule(IR, /*Force=*/true);
+assert((M && >getContext() == ) && "Missing/Mismatching Module");
+std::error_code EC;
+raw_fd_ostream OS(OptBisectPrintIRPath, EC);
+if (EC)
+  report_fatal_error(errorCodeToError(EC));
+M->print(OS, nullptr);
+  }
+  return 

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Evgeniy via Phabricator via cfe-commits]

ebrevnov updated this revision to Diff 477706.
ebrevnov added a comment.
Herald added a project: Flang.

Fixed build error in flang/lib/Frontend/FrontendActions.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  flang/lib/Frontend/FrontendActions.cpp
  llvm/include/llvm/IR/OptBisect.h
  llvm/include/llvm/Passes/StandardInstrumentations.h
  llvm/lib/Analysis/CallGraphSCCPass.cpp
  llvm/lib/Analysis/LoopPass.cpp
  llvm/lib/Analysis/RegionPass.cpp
  llvm/lib/IR/LLVMContextImpl.cpp
  llvm/lib/IR/OptBisect.cpp
  llvm/lib/IR/Pass.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Passes/PassBuilderBindings.cpp
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/IR/LegacyPassManagerTest.cpp
  llvm/unittests/IR/PassManagerTest.cpp

Index: llvm/unittests/IR/PassManagerTest.cpp
===
--- llvm/unittests/IR/PassManagerTest.cpp
+++ llvm/unittests/IR/PassManagerTest.cpp
@@ -826,7 +826,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
@@ -872,7 +872,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
@@ -937,7 +937,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
Index: llvm/unittests/IR/LegacyPassManagerTest.cpp
===
--- llvm/unittests/IR/LegacyPassManagerTest.cpp
+++ llvm/unittests/IR/LegacyPassManagerTest.cpp
@@ -359,10 +359,8 @@
 struct CustomOptPassGate : public OptPassGate {
   bool Skip;
   CustomOptPassGate(bool Skip) : Skip(Skip) { }
-  bool shouldRunPass(const Pass *P, StringRef IRDescription) override {
-if (P->getPassKind() == PT_Module)
-  return !Skip;
-return OptPassGate::shouldRunPass(P, IRDescription);
+  bool shouldRunPass(const StringRef PassName, StringRef IRDescription) override {
+return !Skip;
   }
   bool isEnabled() const override { return true; }
 };
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -354,8 +354,8 @@
   PrintPassOptions PrintPassOpts;
   PrintPassOpts.Verbose = DebugPM == DebugLogging::Verbose;
   PrintPassOpts.SkipAnalyses = DebugPM == DebugLogging::Quiet;
-  StandardInstrumentations SI(DebugPM != DebugLogging::None, VerifyEachPass,
-  PrintPassOpts);
+  StandardInstrumentations SI(M.getContext(), DebugPM != DebugLogging::None,
+  VerifyEachPass, PrintPassOpts);
   SI.registerCallbacks(PIC, );
   DebugifyEachInstrumentation Debugify;
   DebugifyStatsMap DIStatsMap;
Index: llvm/lib/Passes/StandardInstrumentations.cpp
===
--- llvm/lib/Passes/StandardInstrumentations.cpp
+++ llvm/lib/Passes/StandardInstrumentations.cpp
@@ -767,27 +767,35 @@
   return ShouldRun;
 }
 
-void OptBisectInstrumentation::registerCallbacks(
+bool OptPassGateInstrumentation::shouldRun(StringRef PassName, Any IR) {
+  if (isIgnored(PassName))
+return true;
+
+  bool ShouldRun =
+  Context.getOptPassGate().shouldRunPass(PassName, getIRName(IR));
+  if (!ShouldRun && !this->HasWrittenIR && !OptBisectPrintIRPath.empty()) {
+// FIXME: print IR if limit is higher than number of opt-bisect
+// invocations
+this->HasWrittenIR = true;
+const Module *M = unwrapModule(IR, /*Force=*/true);
+assert((M && >getContext() == ) && "Missing/Mismatching Module");
+std::error_code EC;
+raw_fd_ostream OS(OptBisectPrintIRPath, EC);
+if (EC)
+  report_fatal_error(errorCodeToError(EC));
+M->print(OS, nullptr);
+  }
+  return ShouldRun;
+}
+
+void OptPassGateInstrumentation::registerCallbacks(
 PassInstrumentationCallbacks ) {
-  if 

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Arthur Eubanks via Phabricator via cfe-commits]

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

the precommit bot is still complaining that 
flang/lib/Frontend/FrontendActions.cpp needs to be updated

lgtm if the precommit bot doesn't show anything related to this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Evgeniy via Phabricator via cfe-commits]

ebrevnov updated this revision to Diff 477446.
ebrevnov marked an inline comment as done.
ebrevnov added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/IR/OptBisect.h
  llvm/include/llvm/Passes/StandardInstrumentations.h
  llvm/lib/Analysis/CallGraphSCCPass.cpp
  llvm/lib/Analysis/LoopPass.cpp
  llvm/lib/Analysis/RegionPass.cpp
  llvm/lib/IR/LLVMContextImpl.cpp
  llvm/lib/IR/OptBisect.cpp
  llvm/lib/IR/Pass.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Passes/PassBuilderBindings.cpp
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/IR/LegacyPassManagerTest.cpp
  llvm/unittests/IR/PassManagerTest.cpp

Index: llvm/unittests/IR/PassManagerTest.cpp
===
--- llvm/unittests/IR/PassManagerTest.cpp
+++ llvm/unittests/IR/PassManagerTest.cpp
@@ -826,7 +826,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
@@ -872,7 +872,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
@@ -937,7 +937,7 @@
   FunctionAnalysisManager FAM;
   FunctionPassManager FPM;
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(/*DebugLogging*/ true);
+  StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ true);
   SI.registerCallbacks(PIC, );
   FAM.registerPass([&] { return PassInstrumentationAnalysis(); });
   FAM.registerPass([&] { return DominatorTreeAnalysis(); });
Index: llvm/unittests/IR/LegacyPassManagerTest.cpp
===
--- llvm/unittests/IR/LegacyPassManagerTest.cpp
+++ llvm/unittests/IR/LegacyPassManagerTest.cpp
@@ -359,10 +359,8 @@
 struct CustomOptPassGate : public OptPassGate {
   bool Skip;
   CustomOptPassGate(bool Skip) : Skip(Skip) { }
-  bool shouldRunPass(const Pass *P, StringRef IRDescription) override {
-if (P->getPassKind() == PT_Module)
-  return !Skip;
-return OptPassGate::shouldRunPass(P, IRDescription);
+  bool shouldRunPass(const StringRef PassName, StringRef IRDescription) override {
+return !Skip;
   }
   bool isEnabled() const override { return true; }
 };
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -354,8 +354,8 @@
   PrintPassOptions PrintPassOpts;
   PrintPassOpts.Verbose = DebugPM == DebugLogging::Verbose;
   PrintPassOpts.SkipAnalyses = DebugPM == DebugLogging::Quiet;
-  StandardInstrumentations SI(DebugPM != DebugLogging::None, VerifyEachPass,
-  PrintPassOpts);
+  StandardInstrumentations SI(M.getContext(), DebugPM != DebugLogging::None,
+  VerifyEachPass, PrintPassOpts);
   SI.registerCallbacks(PIC, );
   DebugifyEachInstrumentation Debugify;
   DebugifyStatsMap DIStatsMap;
Index: llvm/lib/Passes/StandardInstrumentations.cpp
===
--- llvm/lib/Passes/StandardInstrumentations.cpp
+++ llvm/lib/Passes/StandardInstrumentations.cpp
@@ -766,27 +766,35 @@
   return ShouldRun;
 }
 
-void OptBisectInstrumentation::registerCallbacks(
+bool OptPassGateInstrumentation::shouldRun(StringRef PassName, Any IR) {
+  if (isIgnored(PassName))
+return true;
+
+  bool ShouldRun =
+  Context.getOptPassGate().shouldRunPass(PassName, getIRName(IR));
+  if (!ShouldRun && !this->HasWrittenIR && !OptBisectPrintIRPath.empty()) {
+// FIXME: print IR if limit is higher than number of opt-bisect
+// invocations
+this->HasWrittenIR = true;
+const Module *M = unwrapModule(IR, /*Force=*/true);
+assert((M && >getContext() == ) && "Missing/Mismatching Module");
+std::error_code EC;
+raw_fd_ostream OS(OptBisectPrintIRPath, EC);
+if (EC)
+  report_fatal_error(errorCodeToError(EC));
+M->print(OS, nullptr);
+  }
+  return ShouldRun;
+}
+
+void OptPassGateInstrumentation::registerCallbacks(
 PassInstrumentationCallbacks ) {
-  if