This is an archive of the discontinued LLVM Phabricator instance.

[tablegen][globalisel] Add support for nested instruction matching.
ClosedPublic

Authored by dsanders on Mar 2 2017, 8:50 AM.

Details

Summary

Lift the restrictions that prevented the tree walking introduced in the
previous change and add support for patterns like:

(G_ADD (G_MUL (G_SEXT $src1), (G_SEXT $src2)), $src3) -> SMADDWrrr $dst, $src1, $src2, $src3

Also adds support for G_SEXT and G_ZEXT to support these cases.

One particular aspect of this that I should draw attention to is that I've
tried to be overly conservative in determining the safety of matches that
involve non-adjacent instructions and multiple basic blocks. This is intended
to be used as a cheap initial check and we may add a more expensive check in
the future. The current rules are:

  • Reject if any instruction may load/store (we'd need to check for intervening memory operations.
  • Reject if any instruction has implicit operands.
  • Reject if any instruction has unmodelled side-effects.

See isObviouslySafeToFold().

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Mar 2 2017, 8:50 AM
dsanders updated this revision to Diff 90348.Mar 2 2017, 9:27 AM

A small improvement to the isObviouslySafeToFold() check. There's no need to
check the root instruction since it isn't moving.

kristof.beyls added inline comments.Mar 7 2017, 7:15 AM
test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-muladd.mir
1–3 ↗(On Diff #90348)

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.

5–6 ↗(On Diff #90348)

I guess these comment lines can be dropped, given this is a test file specifically for muladd patterns?

utils/TableGen/GlobalISelEmitter.cpp
712 ↗(On Diff #90348)

The other derived classes have a comment explaining what their intent roughly is. This one probably also should have a comment.

1019–1052 ↗(On Diff #90348)

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?

dsanders added inline comments.Mar 7 2017, 5:32 PM
test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-muladd.mir
1–3 ↗(On Diff #90348)

Ok, I'll cut this down to one RUN line.

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

5–6 ↗(On Diff #90348)

Yep :-). I've been copying the main test and editing it from there and have forgotten about the comments.

utils/TableGen/GlobalISelEmitter.cpp
712 ↗(On Diff #90348)

Ok.

1019–1052 ↗(On Diff #90348)

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.

dsanders updated this revision to Diff 91057.Mar 8 2017, 11:17 AM

Add requested comments and test case. Removed unnecessary RUN lines and out of date comments.

dsanders marked 8 inline comments as done.Mar 8 2017, 11:23 AM

Add requested comments and test case. Removed unnecessary RUN lines and out of date comments.

Nearly forgot to mention it, also fixed an off-by-one error where it wouldn't call isObviouslySafeToFold() for two-insn patterns.

Thanks for the updates, Daniel!
I don't have further remarks, but I think it's best if someone who understands the details better (like @ab ?) would also have a look.

test/TableGen/GlobalISelEmitter.td
131 ↗(On Diff #91057)

Thanks very much for this test. I find the test also works well as documentation for what the expected generated code is for a simple piece of tablegen input.
I'm assuming that this is the single test file that drives code coverage statistics at http://llvm.org/reports/coverage/utils/TableGen/GlobalISelEmitter.cpp.gcov.html. I was already happily surprised to see so much line coverage there. It seems only error reporting isn't well covered by this test file.

igorb added a subscriber: igorb.Mar 21 2017, 12:32 AM
dsanders updated this revision to Diff 92920.Mar 24 2017, 4:37 AM

Refresh and ping.
No longer selects code for dead intermediates due to r298224

dsanders edited the summary of this revision. (Show Details)Mar 24 2017, 4:40 AM
rovka added inline comments.Mar 31 2017, 8:16 AM
lib/Target/AArch64/AArch64InstructionSelector.cpp
938 ↗(On Diff #92920)

Aren't these handled by TableGen now?

test/CodeGen/AArch64/GlobalISel/select-xor.mir
1 ↗(On Diff #92920)

Why is this change necessary?

utils/TableGen/GlobalISelEmitter.cpp
289 ↗(On Diff #92920)

Any reason why this needs to be [...]ForOperand instead of just emitCxxCaptureStmts?

655 ↗(On Diff #92920)

This TODO should be obsolete now.

dsanders added inline comments.Mar 31 2017, 8:31 AM
lib/Target/AArch64/AArch64InstructionSelector.cpp
938 ↗(On Diff #92920)

Good point. I've forgotten to delete the C++ implementation

test/CodeGen/AArch64/GlobalISel/select-xor.mir
1 ↗(On Diff #92920)

I think this appeared as a result rebasing over an upstream change but it shows up in my local copy of this patch too. I'll undo it.

utils/TableGen/GlobalISelEmitter.cpp
289 ↗(On Diff #92920)

I don't recall a good reason for this. I think I did it to make it easier to navigate around the source.

655 ↗(On Diff #92920)

Thanks for noticing this. I'll remove it.

ab accepted this revision.Mar 31 2017, 10:57 AM

Assuming Kristof and Diana agree, LGTM with Diana's comments (and a couple more of my own) addressed.

test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-muladd.mir
1 ↗(On Diff #92920)

Rename the tests to match the others, "test/CodeGen/AArch64/select-*.mir" ?

10 ↗(On Diff #92920)

Stale comment?

test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-sext.mir
6 ↗(On Diff #92920)

We already have select-int-ext.mir. Can you keep the tests together? (there are enough to keep them in separate files now, though)

12 ↗(On Diff #92920)

-> sext_s32_s16_gpr to match the operand order? (and more importantly, the existing tests!)

test/CodeGen/AArch64/GlobalISel/select-xor.mir
1 ↗(On Diff #92920)

Can you fix the new tests as well?

utils/TableGen/GlobalISelEmitter.cpp
289 ↗(On Diff #92920)

Can you undo it?

This revision is now accepted and ready to land.Mar 31 2017, 10:57 AM
ab added inline comments.Mar 31 2017, 11:05 AM
test/TableGen/GlobalISelEmitter.td
85 ↗(On Diff #92920)

On second thought, I'm not sure this is sufficient: don't we need to merge the memops?

dsanders marked 13 inline comments as done.Apr 4 2017, 5:12 AM
dsanders added inline comments.
lib/Target/AArch64/AArch64InstructionSelector.cpp
938 ↗(On Diff #92920)

It turns out that these are still doing something useful. They're needed for the sext_inreg/zext_inreg operator from SelectionDAG since this hasn't been mapped to GlobalISel yet

test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-sext.mir
6 ↗(On Diff #92920)

I've merged these tests in with the recently-added select-int-ext.mir tests.

12 ↗(On Diff #92920)

Both orders make sense from different perspectives (mine was derived from LLVM-IR's sext/zext instruction) but I've renamed mine to match the ones that were recently committed.

test/TableGen/GlobalISelEmitter.td
85 ↗(On Diff #92920)

I spotted another issue with this yesterday. It's unusual but some targets may choose to lower a load to a non-load and in this case we shouldn't copy the memory operands across otherwise the MachineVerifier will object.

I'll have a quick look at merging the operands.

This revision was automatically updated to reflect the committed changes.
dsanders marked 2 inline comments as done.
dsanders added inline comments.Apr 4 2017, 6:39 AM
test/TableGen/GlobalISelEmitter.td
85 ↗(On Diff #92920)

I've made a minor change to make it merge memops from all input instructions and committed.