Page MenuHomePhabricator

[GlobalISel] Generate selector with predicates; use it for FP binops.
Needs ReviewPublic

Authored by ab on Feb 2 2017, 4:56 PM.

Details

Summary

This emits (rule-level) checks for pattern predicates. That mostly includes
subtarget feature checks. This lets us emit a selector for basic FP binops.

Using all predicates as-is in the selector emitter is not going to be
trivial in general: predicates can be arbitrary code (running in
a <Target>DAGToDAGISel member), so, over time, we've grown some pretty
interesting uses.

In the long run, we'll want to either:

  • add first-class support for the different families of predicates
  • add some mappings from DAGToDAGISel code to the GISel equivalent

Or a combination of both.

I originally tried to express each Predicate as a RuleMatcher
predicate child, but, without the extra checking and grouping of
predicate families, that just looked like noisy boilerplate.

In the meantime, use the aggregate raw predicate check code string.

Diff Detail

Event Timeline

ab created this revision.Feb 2 2017, 4:56 PM
kristof.beyls added inline comments.Feb 3 2017, 4:35 AM
utils/TableGen/GlobalISelEmitter.cpp
366–368

I've gotten the feedback that most developers don't really understand how the auto-generated "magic" works for DAGISel.
So far, I think the current design is pretty easy to understand in comparison. I've tried being picky in this area before about things like naming to keep the code as-easy-to-understand as possible.
Maybe the 3 variables here could use a short comment in that respect?
Maybe something like

/// optional code string that indicates when this rule is legal
Optional<std::string> Predicate;
/// list of matchers that all need to evaluate true for the current IR node to match pattern P.
std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
/// re-writing actions that need to be taken when the current IR node matches pattern P and Predicate is true.
std::vector<std::unique_ptr<MatchAction>> Actions;

Please note that I haven't tried hard to make the comment fully factual correct - I'm hoping Daniel or Ahmed will be able to quickly make those comments factually correct.

392–408

I think it's slightly easier to read if the assert would go before any of the code-emitting code.
It makes it slightly easier to read what code gets generated.

dsanders edited edge metadata.Feb 3 2017, 8:04 AM

This needs a testcase.

utils/TableGen/GlobalISelEmitter.cpp
366

I'd prefer it if we did these predicates the same way as for Instructions and Operands (i.e. with PredicateListMatcher<RulePredicateMatcher>). This is because I'm trying to push the SelectionDAG dependency out of the *Matcher classes so we can eventually import pure-GlobalISel rules in addition to the imported SelectionDAG rules. I also expect we'll eventually have GlobalISel-specific predicates that control when rules are applied (e.g. ISel, Pseudo-expansion, etc.)

Reaching that point isn't a near term goal though so I don't have any strong objections to importing the whole predicate expression for now and making it more precise later

366–368

These comments are accurate. I'd add a FIXME saying that Matchers is only allowed to be 1-element long for now until we have a way to match multiple independent instructions.

ab updated this revision to Diff 87053.Feb 3 2017, 5:20 PM
ab marked 4 inline comments as done.

WIP: only recognize predicates that map to Subtarget features (the unfortunately named AssemblerPredicate). Those should always be supported.

I grabbed the feature flags off of the subtarget. We should do something similar to the asm matcher and cache the combined feature flags. I'll look into deduplicating the various computeAvailableFeatures instances; we could also generate that code in the selector itself. Either way, that gives us support for flag logic for free (e.g., "FeatureFoo,!FeatureBar").

In the meantime, what do you folks think?

ab added a comment.Feb 3 2017, 5:26 PM

This needs a testcase.

Right! I've been wanting to write tablegen tests forever but never remember. I guess it just doesn't come naturally in these parts ;) Done in r294075.

utils/TableGen/GlobalISelEmitter.cpp
366

Yeah, that's what I originally did, but it felt quite boilerplate-y, so I figured we keep it simple in the meantime. Either works though.

366–368

Done with a few tweaks in r294077, let me know if you spot anything else!

392–408

Fair enough; done.

In D29478#666554, @ab wrote:

This needs a testcase.

Right! I've been wanting to write tablegen tests forever but never remember. I guess it just doesn't come naturally in these parts ;) Done in r294075.

Sorry for being ambiguous here. I meant a test case that checks the output for G_FADD and the other newly-added FPU operations but having just checked test/CodeGen/AArch64/GlobalISel/arm64-instructionselect.mir it seems we're re-using some existing tests from when they were added to the C++ implementation.

Thanks for adding tablegen tests. I'll make sure my patches update them too.

WIP: only recognize predicates that map to Subtarget features (the unfortunately named AssemblerPredicate). Those should always be supported.

I grabbed the feature flags off of the subtarget. We should do something similar to the asm matcher and cache the combined feature flags.
I'll look into deduplicating the various computeAvailableFeatures instances; we could also generate that code in the selector itself. Either way,
that gives us support for flag logic for free (e.g., "FeatureFoo,!FeatureBar").

In the meantime, what do you folks think?

IIRC, SelectionDAG only uses the Predicate class and the MC layer only uses AssemblerPredicate. It's probably safest to stick to that but there's some advantages to the AssemblerPredicate way so it makes sense to make use that approach in GlobalISel where possible. The main one I see is that it's possible to test multiple predicates simultaneously. If we do use AssemblerPredicates in GlobalISel, we shouldn't continue to call it AssemblerPredicate though.

I should mention that AArch64 has a few Predicates (e.g. IsLE) that don't have an AssemblerPredicate. We'll need to fix that. Also, Mips has an AssemblerPredicate that lacks a Predicate because it only makes sense to the assembly (UseTCCInDiv).

ab added a comment.Feb 16 2017, 10:04 AM
In D29478#666554, @ab wrote:

In the meantime, what do you folks think?

IIRC, SelectionDAG only uses the Predicate class and the MC layer only uses AssemblerPredicate. It's probably safest to stick to that but there's some advantages to the AssemblerPredicate way so it makes sense to make use that approach in GlobalISel where possible. The main one I see is that it's possible to test multiple predicates simultaneously. If we do use AssemblerPredicates in GlobalISel, we shouldn't continue to call it AssemblerPredicate though.

You're right on all points. I was thinking we could split AssemblerPredicate into something like SubtargetFeaturePredicate and AssemblerPredicate: what's interesting to us is that AssemblerPredicates only need an MCSubtargetInfo. SDAG Predicates have arbitrarily weird code that we will never be able to support in the general case.

I should mention that AArch64 has a few Predicates (e.g. IsLE) that don't have an AssemblerPredicate. We'll need to fix that. Also, Mips has an AssemblerPredicate that lacks a Predicate because it only makes sense to the assembly (UseTCCInDiv).

These Predicates should probably also have a way of defining GISel equivalence.

dsanders resigned from this revision.Thu, Jul 18, 6:51 PM