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

Tue Dec 13 16:07:12 PST 2016

ab added a comment.

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?)
How is that expressed in the current backend?  Are operations that map to non-sign-extending instructions (if there are any?) explicitly legalized?

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.

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?

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

================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:99
+
+struct MatchAction {
+  virtual ~MatchAction() {}
----------------
dsanders wrote:
> This is a nitpick but I think we should keep Matchers and Actions in separate class hierarchies for type checking purposes. They don't share much implementation and it doesn't make sense to put MatchOpcode in Matcher::Actions or MutateOpcode in Matcher::Matchers
Fair enough; split it into 'Matcher' and 'MatchAction', and renamed 'Matcher' to 'MatcherEmitter'.  Better names welcome ;)

================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:122
+  unsigned OpIdx;
+  MVT::SimpleValueType VT;
+  bool Widenable;
----------------
dsanders wrote:
> We don't use the MVT directly. Would it be better to store the LLT and convert in the caller?
You're right, MVT doesn't really belong here, but the string is quite wasteful.  Ideally, we'd use LLT itself (or fake it here), but that would have to move to libSupport, which seems gross.

Anyway, switched to a std::string LLT.

https://reviews.llvm.org/D26878