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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders 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
----------------
kristof.beyls wrote:
> 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.
Ok, I'll cut this down to one RUN line.

It also applies to the xor one from an earlier patch.


================
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.
+
----------------
kristof.beyls wrote:
> I guess these comment lines can be dropped, given this is a test file specifically for muladd patterns?
Yep :-). I've been copying the main test and editing it from there and have forgotten about the comments.


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


================
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
----------------
kristof.beyls wrote:
> 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?
We have test/TableGen/GlobalISelEmitter.td but I'll add something there. However, it won't be able to test the detail of this comment since it's talking about the requirements of a more precise version of isObviouslySafeToFold().

The testing for the detail of this fixme is best done in test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-*.mir by adding some unsafe cases and testing that they don't match.


https://reviews.llvm.org/D30539