Page MenuHomePhabricator

[gicombiner] Allow generated combiners to store additional members
ClosedPublic

Authored by dsanders on Jun 15 2020, 12:08 PM.

Details

Summary

Adds the ability to add members to a generated combiner via
a State base class. In the current AArch64PreLegalizerCombiner
this is used to make Helper available without having to
provide it to every call.

As part of this, split the command line processing into a
separate object so that it still only runs once even though
the generated combiner is constructed more frequently.

Depends on D81862

Diff Detail

Event Timeline

dsanders created this revision.Jun 15 2020, 12:08 PM
arsenm added inline comments.Jun 15 2020, 12:13 PM
llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp
58

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?

arsenm added inline comments.Jun 15 2020, 12:22 PM
llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp
58

I guess this does let you choose your member name

dsanders marked an inline comment as done.Jun 15 2020, 1:05 PM
dsanders added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp
58

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

arsenm accepted this revision.Jun 15 2020, 1:15 PM
This revision is now accepted and ready to land.Jun 15 2020, 1:15 PM
llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerCombiner.cpp
298

These names are getting a bit too long. Can we come up with a better naming scheme for these?

llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp
77

Similar with how long this is?

dsanders marked an inline comment as done.Jun 15 2020, 3:12 PM
dsanders added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerCombiner.cpp
298

I agree but it's written exactly once so I'm not too worried about it at the moment. It could become a problem if we start passing them around though. I might make it a class-scoped class at some point but that won't really make it any shorter in this particular definition (actually it will add :: to it).

Part of the current name is user selected so it's really ${prefix}RuleConfig. Maybe tablegen should let you pick both names (that would prevent class-scoping it but that's not much of a loss).

This revision was automatically updated to reflect the committed changes.
dsanders marked an inline comment as not done.