[PATCH] D35117: [globalisel][tablegen] Add control-flow to the MatchTable.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:33
+  enum { RejectAndGiveUp, RejectAndResume };
+  auto handleReject = [&]() {
+    DEBUG(dbgs() << CurrentIdx << ": Rejected\n");
----------------
rovka wrote:
> I think omitting the return type for a lambda with more than one return statement is C++14, you might run into trouble with some of the buildbots.
You're right. Apparently some compilers will silently accept it but I expect there will be some that don't. I'll fix this.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:237
     }
+    case GIM_Reject:
+      DEBUG(dbgs() << CurrentIdx << ": GIM_Reject");
----------------
rovka wrote:
> Both the comment on this and the summary say that this either exists the current try block or completely fails to match if there is no try block. That's not what this does, this just completely fails. Your lambda seems to take care of the logic for exiting the current block or bailing out, so it seems like this state is really unnecessary - it would be enough to just not generate a Try block at all for the last instruction that you're matching (so you wouldn't actually need to generate a Try/Reject pair for any of the current tests). Am I missing something?
Good point. I think I missed this one because at the moment it only appears as the last opcode of a table to prevent executeMatchTable() from falling off the end.

I'm going to need it inside GIM_Try blocks once I start nesting them so I'll correct this one to use handleReject() too.

> it would be enough to just not generate a Try block at all for the last instruction that
> you're matching (so you wouldn't actually need to generate a Try/Reject pair for any of
> the current tests)

I'd prefer not to special case the last block since this will get quite confusing once GIM_Try blocks start nesting. I think it's an optimization we should leave until late in development if we do it.


================
Comment at: test/TableGen/GlobalISelEmitter.td:100
+// CHECK-NEXT:    GIM_Try, /* On fail goto */ /* Label 0 */ [[LABEL:[0-9]+]],
+// CHECK-NEXT:    GIM_CheckNumOperands, /* MI */ 0, /* Expected */ 4,
+// CHECK-NEXT:    GIM_CheckOpcode, /* MI */ 0, TargetOpcode::G_SELECT,
----------------
rovka wrote:
> The spaces in the comments are really unrelated, can you get rid of them for this patch?
Sure, they're part of the formatting that MatchTable::Comment() so I can make that format without the spaces.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:234
+/// A record to be stored in a MatchTable.
+/// This class supports all kinds of data that needs to be stored in the data.
+struct MatchTableRecord {
----------------
rovka wrote:
> This comment sounds weird, can you rephrase?
The last word was supposed to be 'table' but it's still not particularly clear after that either.

The main thing I'm trying to convey is that there's no inheritance hierarchy (the MatchTableRecord class supports all the quirks of each kind of record such as adding comment characters and indenting) so that the table can be a simple vector. How about this:
  This class represents any and all output that may be required to emit the MatchTable. Instances  are most often configured to represent an opcode or value that will be emitted to the table with some formatting but it can also represent commas, comments, and other formatting instructions.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:255
+    /// record.
+    MTRF_Outdent = 0x40,
+  };
----------------
rovka wrote:
> How do the other TableGen backends handle indentation? Personally I preferred your previous patch which just ran clang-format on the generated files, since that kept the backends simple. But if other people don't like that approach, we should at least try to share some of this stuff between backends (if it wouldn't be too convoluted).
Most of them do it on an ad-hoc basis when they need it and hard-code the whitespace if they don't need anything special. The closest example is SelectionDAG which indents its table in a similar way so that it's easy for a human to see the structure of the decisions being made by the state machine.

Unfortunately, the clang-format approach won't help in this case because it's not aware of the meaning of the data. It's not aware that it ought to keep opcodes and arguments on the same line or that GIM_Try/GIM_Reject should affect indentation to help a human follow the decisions the state machine will make. It would be neat if it was possible to teach clang-format about special cases like this table but that will be more complicated.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:454
   /// A map of instruction matchers to the local variables created by
-  /// emitCxxCaptureStmts().
+  /// emitCaptureOpcodes().
   std::map<const InstructionMatcher *, unsigned> InsnVariableIDs;
----------------
rovka wrote:
> Unrelated change, please commit separately.
Ok


https://reviews.llvm.org/D35117