[PATCH] D48545: [RFC v2] "Alternative" matches for TableGen DAG patterns

[Hal Finkel via Phabricator via llvm-commits]

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

LGTM (there are a lot of changes here, but given that it produces no changes to existing matching tables, that seems like pretty good test coverage).

> As a follow-on, we also need to update the two other places that currently still refer to the full instruction pattern (including the "set" and "implicit" nodes): these are InferFromPattern and EmitResultInstructionAsOperand. The former currently infers flags separately from Instruction and Pattern nodes; now it just uses the PatternToMatch list, which contains both. This required rewriting the way the "isBitcast" flag is inferred. The latter uses the instruction pattern to search for SDNPHasChain property flags. This is now moved to InferFromPattern as well (since it basically is the same operation).



> Note that in order to avoid spurious changes to other targets, I've for now still restricted the new inference code for the isBitcast and hasChain flags to patterns that originated from Instruction nodes. In general, it would probably be better to remove this final difference between instruction patterns and "regular" patterns, but that should IMO only be done after analysing the specific impacts on existing targets, so this should be done as a follow-on patch.

I definitely encourage resolving these in follow up. I think it will be better if this works uniformly.

> 1. I've renamed the PatFrag class to PatFrags, holding a "list<dag> Fragments" instead of a "dag Fragment". Does the name make sense? Or should this be called something like MultiPatFrag, or PatFragAlternatives, or ...?

I think that PatFrags is good.

> 2. The original PatFrag is now a subclass of PatFrags, specialized to a single fragment. This required some changes to two targets (Hexagon and NVPTX) that directly accessed implementation details of PatFrag (specifically, the "Fragment" member). Should we instead leave PatFrag as is and and a nonrelated PatFrags class? This would make the implementation in TableGen a bit wordier ...

I think that it should be logically organized as you have in the patch. After this goes in, we should make sure to update the release notes and send a "heads up" email to llvm-dev for out-of-tree targets with instructions on how to update.

> 3. Theoretically, a PatFrags instance can now have an empty Fragments list -- this would simply never match anything. Should this be explicitly forbidden? On the other hand, we might also use this to implement "null_frag" without hardcoding its name in TableGen ...

I don't see any reason to disallow it. Getting rid of null_frag as a special case seems nice.



================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:3624
 
+  // FIXME: Assume only the first tree is the pattern. The others are clobber
+  // nodes.
----------------
Are you saying that, if we have clobber nodes after the pattern, then this will mistakenly take the last clobber node as the pattern?


Repository:
  rL LLVM

https://reviews.llvm.org/D48545