[PATCH] D82344: [NewPM] Make PMs and adaptor passes for PMs unskippable

[Yuanfang Chen via Phabricator via llvm-commits]

ychen marked an inline comment as done.
ychen added a comment.

In D82344#2144872 <https://reviews.llvm.org/D82344#2144872>, @aeubanks wrote:

> I think I prefer https://reviews.llvm.org/D83575 over this, this uses too much template metaprogramming for my liking. WDYT?


I'm happy if either this or D83575 <https://reviews.llvm.org/D83575> are landed and slightly in favor of this. The difference between this and D83575 <https://reviews.llvm.org/D83575> is that if we want the pass to implement an extra `require` method all the time. I choose not too because I think it is less pervasive and I imagine the use cases for `require` are limited. I'm kind of want to hide this detail from most pass developers so they don't need to worry about this in most cases. The template stuff is used so we don't mandate a `require` method all the time and the templates could be simplified using `is_detected`.

Other than the points I made I think it is perfectly reasonable to pursue D83575 <https://reviews.llvm.org/D83575>. For one thing, I'd prefer `require` over `skippable`. And it requires less code diff FWIW.

Anyway since we're authors of the alternatives and I'm probably biased. It would be a good thing to solicit other opinions. Thoughts?



================
Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:359
+  static bool isSkippable() {
+    return !std::is_base_of<CGSCCPassT, CGSCCPassManager>::value;
+  }
----------------
aeubanks wrote:
> This is saying that only a ModuleToPostOrderCGSCCPassAdaptor around a CGSCCPassManager isn't skippable, all other ModuleToPostOrderCGSCCPassAdaptor are? What about a wrapper around a normal CGSCC pass that isn't skippable?
> What about a wrapper around a normal CGSCC pass that isn't skippable?

Good point! I was not considering the case of normal passes declaring `require`. I think it should return false unconditionally here to mean the adaptor itself is always `required`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82344