[PATCH] D81863: [gicombiner] Allow generated combiners to store additional members

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked an inline comment as done.
dsanders added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp:58
+protected:
+  CombinerHelper &Helper;
+
----------------
arsenm wrote:
> arsenm wrote:
> > Should every combiner just get this by default? I would think every combiner pass would use this, and this just adds more boilerplate to every target?
> I guess this does let you choose your member name
It also allows you to choose the type or add more context. One example of this is that downstream we're going to have a TgtHelper that contains our target specific combines and start moving the more generic parts of it upstream. I'm not currently sure whether that's as well as having Helper or if we can just change the type to our target specific one and still call it Helper.

Another example, is it allows targets to share code between different combiner phases while still allowing the phases to differ. Probably the most common form of this will be a pre/post legalizer predicate so that pre-legalize can simplify as far as possible, while post-legalize enforces additional predicates (or takes additional steps) to ensure legality is preserved.

If we were to have a target independent set for combiners I'd do it via a base class for the State class so that it's possible to remove things too. I wouldn't hardcode it because there's also the potential future uses to consider, using this in other passes (especially target specific ones) where having a CombinerHelper imposed makes less sense. At this point, each combiner that just uses the default could just have:
```
  let StateClass = "CombinerHelperState";
```
(or that could be the default), those that only want to extend can subclass `CombinerHelperState` and provide their subclasses name, and finally those that want to omit things can provide their own class that doesn't subclass `CombinerHelperState`.

I don't mind adding a follow up patch that unifies the targets to a default state class


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81863