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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In https://reviews.llvm.org/D48545#1160649, @hfinkel wrote:

> 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).


Thanks for the review!

>> 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.

OK, I'll work on this as a follow-up.

>> 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.

OK, I'll continue to allow empty Fragments.  Unfortunately, getting rid of null_frag is more difficult than I expected: since current code checks for null_frag so very early, many backends get away with patterns that are actually invalid even structurally (number of results don't match number of users; use of some TableGen operators inside a pattern that aren't even dag nodes at all) as long as there's a null_frag anywhere in there as well.  Trying to define a null_frag as a PatFrags with empty fragment list means that TableGen would still ignore the pattern, but only after doing a structural parse first -- which now errors out.

Now, possibly we should still do it, and clean up those backends first.  But that again should be a follow-on patch.

> Are you saying that, if we have clobber nodes after the pattern, then this will mistakenly take the last clobber node as the pattern?

First of all, note that I just copied this piece of code including its FIXME from one place to another, so my patch doesn't change anything substantive here.

The actual problem that the FIXME calls out is simply this: An original instruction can be a list including (set) statements, (implicit) statements, and plain source patterns (like e.g. stores).  Now, **some** code is currently written as if to allow any sequence of any of these (e.g. more than one set, an implict followed by a set, etc).  But **other** code already imposes a much more strict ordering: the first element must be either a set or a plain source pattern, and any subsequent elements must all be implicits.  The code following the FIXME is one place where this more strict assumption is made.

I assume the reason for calling out a FIXME was that at some point, it was thought that this restriction was unreasonably harsh, and we **should** actually allow more generic sequences?  But as the code stands today, I'd rather take the opposite position: we **should** make the restrictive assumption, because only under this assumption can we actually transform the instruction pattern into a "Pat" style pattern.  (What should it even mean, exactly, to have two set nodes? )  But if this is true, then it would make more sense to fix the other pieces of code, and simply enforce the strict rules everywhere ...


Repository:
  rL LLVM

https://reviews.llvm.org/D48545