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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

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.

The Widenable flag will eventually need to be controllable so that we can say that i1-i63 are widenable except for i32.

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.

> 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. The reason for this, is that I expect we'll eventually want to match multiple instructions with a register-use in common and merge them together and this can't be represented by nesting dags in tablegen. For example, this could be used to match two load-words of the form:

  (set s32_GPR:$dst1, (G_LOAD p0_GPR:$addr))
  (set s32_GPR:$dst2, (G_LOAD (G_ADD p0_GPR:$addr, 4)))

and (subject to other predicates) merge them into a load-word-multiple:

  (set2 s32_GPR:$dst1, s32_GPR32:$dst2, (G_LOAD2 p0_GPR:$addr))



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:99
+
+struct MatchAction {
+  virtual ~MatchAction() {}
----------------
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


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:122
+  unsigned OpIdx;
+  MVT::SimpleValueType VT;
+  bool Widenable;
----------------
We don't use the MVT directly. Would it be better to store the LLT and convert in the caller?


https://reviews.llvm.org/D26878