[PATCH] D26878: [GlobalISel] Add tentative Selector-emitted tablegen backend.

Fri Dec 16 09:31:07 PST 2016

dsanders added a comment.

In https://reviews.llvm.org/D26878#621671, @ab wrote:

> In https://reviews.llvm.org/D26878#613434, @dsanders wrote:
>
> > From https://reviews.llvm.org/differential/diff/78609/:
> >
> >   /// Is this generic instruction "widenable", i.e., is this transform valid:
> >   ///   (G_ZEXT (op s8 a, b)) -> (op s32 (G_ZEXT a), (G_ZEXT b))
> >   
> >
> > This transform isn't strictly valid and will produce the wrong result on overflow but that's often ok because LLVM-IR doesn't define the excess bits and there will be explicit extends for when it really matters. However, some targets like Mips do care what those bits are and may misbehave if the excess bits are wrong. Here's a few examples:
> >
> > - 'addu' where one of the operands is 0x00000001_00000000 produces an unpredictable result on Mips64.
> > - 'addu' where one of the operands is 0x00000000_80000000 produces an unpredictable result on Mips64.
> > - 'addu' and 'daddu' will behave differently when given 0x80000000 and 0x80000000 with the former producing 0x0 and the latter producing 0x1_00000000.
> > - 'addu' and 'daddu' will behave differently when given 0x40000000 and 0x40000000 with the former producing 0xffffffff_80000000 and the latter producing 0x00000000_80000000.
>
>
> That's interesting.. I'm guessing that means that *all* operations that produce a 32-bit result sign-extend it? (usually/always implicitly, as I see 'addu' does?)

Not quite, Mips has some 32-bit zero-extending operations on MIPS64. It's easier to explain the strategy that causes this quirk and work back from there.

The forwards compatibility strategy that the Mips ISA follows views the registers as being infinite-width. Particular versions of the ISA such as MIPS32 choose to implement a certain physical register width and all operations will notionally sign-extend that register-width to infinite-width. When a wider version of the ISA is invented, those notional sign-extensions become real sign-extensions to the new register-width (and notionally beyond to infinite-width). So far that's consistent with your guess, but the wider ISA also adds zero-extending versions of the operations on the previous register-width where it's useful (e.g. 32x32->64-bit unsigned multiplies on MIPS64).

> How is that expressed in the current backend?

There are a couple calling convention legalizations (e.g. to make 'unsigned int' use ISD::ASSERT_SEXT) but it's mostly implicit. Mips just defines the bits that the DAG leaves undefined and people who work on Mips have to be careful not to break the consistency.

> Are operations that map to non-sign-extending instructions (if there are any?) explicitly legalized?

There aren't any non-sign-extending instructions for register-width operations and the instruction selection picks the sign-extending instruction when working on register-width values regardless of the signedness of the operation.

> I don't think 'Widenable' is necessarily a problem: whether the selector decides to implicitly widen is only an isel decision:  the operation still has to be marked legal in the first place.  I think the core problem is that 'Widenable', as implemented in the original patch, introduces ambiguity in the selector.  If we ensure that we only implicitly widen if there is no narrower legal alternative (so, on MIPS64, widen s8 to s32, and s48 to s64, but not s32 to s64), we should guarantee that the selector respects the legalizer's decision.

Both parts of that sound right to me. I expect most targets will be able to defer widening but Mips will need to do it in the legalizer.

> To be clear: the original patch is incomplete in that regard: it will happily select an s32 G_ADD to s64.  Fixing that will require evaluating all patterns before emitting them.  I'll think about it some more and create another thread for that.  It's not obvious we can make it robust (what do we do if there's a legal s32 ADD requiring some predicate?)     For now, let's remove the widening from this.
> 
> We can also remove the concept entirely, and go back to explicitly legalizing.  After all, it is a compile-/run-time optimization!
> 
>> The Widenable flag will eventually need to be controllable so that we can say that i1-i63 are widenable except for i32.
> 
> That's an option, but I'd rather not go down that road ;)
> 
>> Another one that's likely to be surprising is that ISD::TRUNCATE isn't a nop on Mips. It's a sign-extend for i32 (and technically for also i64 but that has no impact at the moment) and a zero extend for other sizes.
> 
> Hmm, how is that represented in SelectionDAG?  Aren't there combines that will try to fold away (aext (trunc x)) or (trunc (zext x)) ?  Wouldn't avoid them require ISD::TRUNCATE be legalized?

It's just a normal ISD::TRUNCATE node without special legalization. There's a special-case instruction selection to emit a sign-extend for the 32-bit case and some folds to handle ISD::ASSERT_SEXT and similar.

Both of those examples are cases where SelectionDAG leaves the bits undefined and Mips defines them to be a sign-extension. In the first one, any extend is an extension using undefined bits so (aext (trunc x)) is equivalent to (trunc x) in terms of register value. Similarly, truncation leaves the truncated bits undefined. They'd normally retain their previous value but Mips changes them to a sign-extension.

>>> invent a new syntax, convert older patterns (at checkin- or build-time): I don't think we're at a stage where we can make
>>>  well-informed decisions about the eventual tablegen design. I'm hoping that, as we add support for SDAG constructs, we
>>>  get more insight on what we'll eventually need. For now, emitting matchers from SDAG patterns is a small incremental
>>>  step.
>> 
>> My current knowledge of GlobalISel is largely based on reading the code rather than writing it but FWIW I've been sketching a rough tablegen design as I've been catching up on GlobalISel and the sketch is broadly in line with this patch.
>> 
>> One thing I'd like to mention is that the current patch has what I'd call rule-level matching (Matcher), and instruction-property-level matching (MatchOpcode/MatchRegOpType/MatchMBBOp). I think we should have a level in between those two which represents an instruction without testing any properties of that instruction. It's probably best to think of it as a cursor position for the match.
> 
> Absolutely!  Unless you had something else in mind, I suspect we'll need something like that even for simple patterns with nested instructions, like this part of your example:
> 
>> (set s32_GPR:$dst2, (G_LOAD (G_ADD p0_GPR:$addr, 4)))
> 
> That alone will require additional context.  For now, I'd rather stick to the absolute simplest starting point.

That's a good point and it nicely unifies the nested match description with the multiple-roots one. The only difference between them is how the match position is found which is a task for the algorithm rather than the description.

https://reviews.llvm.org/D26878