[PATCH] D30539: [tablegen][globalisel] Add support for nested instruction matching.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-muladd.mir:1-3
+# RUN: llc -O0 -mtriple=aarch64-apple-ios -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=IOS
+# RUN: llc -O0 -mtriple=aarch64-linux-gnu -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=LINUX-DEFAULT
+# RUN: llc -O0 -mtriple=aarch64-linux-gnu -relocation-model=pic -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=LINUX-PIC
----------------
I'm not sure if we need all 3 run lines for this specific test?
I don't see and of the IOS, LINUX-DEFAULT or LINUX-PIC check prefixes being used in this test, implying that there is no difference in expected behaviour for these environments, for this test.
Maybe best not to copy paste the 3 run lines into many different tests, to avoid unnecessarily increasing the number of processes needed to run the regression tests?
This question probably applies to the other newly added tests in this patch too.


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-muladd.mir:5-6
+
+# Test the instruction selector.
+# As we support more instructions, we need to split this up.
+
----------------
I guess these comment lines can be dropped, given this is a test file specifically for muladd patterns?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:712
 
+class InstructionOperandMatcher : public OperandPredicateMatcher {
+protected:
----------------
The other derived classes have a comment explaining what their intent roughly is. This one probably also should have a comment.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1019-1052
+      // FIXME: Emit checks to determine it's _actually_ safe to fold and/or
+      //        account for unsafe cases.
+      //
+      //        Example:
+      //          MI1--> %0 = ...
+      //                 %1 = ... %0
+      //          MI0--> %2 = ... %0
----------------
I was looking for a tablegen-regression test to understand the intended tablegen codegen changes, but I didn't find one in this patch. I guess we don't have tablegen backend regression tests yet?
The comment above makes me think that we really should have them, and maybe this patch is about the right time to create that?
I'm thinking about the input for a test being a small piece of tablegen, and the output being checked against being the generated C++ code by this backend.
I think that the complexity of this tablegen backend is slowly becoming too high to test it well using just users of that tablegen backend, like the AArch64 instruction selector.
I think I saw a similar comment in an earlier patch in this area, but I'm not sure what the conclusion was?

Does creating tablegen backend regression tests seem sensible, useful, and needed here?
If there isn't a framework yet for creating such tests, could you use some help in trying to create an initial framework for such tests?


https://reviews.llvm.org/D30539