This is an archive of the discontinued LLVM Phabricator instance.

[tablegen][globalisel] Capture instructions into locals and related infrastructure for multiple instructions matches.
ClosedPublic

Authored by dsanders on Mar 2 2017, 6:55 AM.

Details

Summary

Prepare the way for nested instruction matching support by having actions
like CopyRenderer look up operands in the RuleMatcher rather than a
specific InstructionMatcher. This allows actions to reference any operand
from any matched instruction.

It works by checking the 'shape' of the match and capturing
each matched instruction to a local variable. If the shape is wrong
(not enough operands, leaf nodes where non-leafs are expected, etc.), then
the rule exits early without checking the predicates. Once we've captured
the instructions, we then test the predicates as before (except using the
local variables). If the match is successful, then we render the new
instruction as before using the local variables.

It's not noticable in this patch but by the time we support multiple
instruction matching, this patch will also cause a significant improvement
to readability of the emitted code since
MRI.getVRegDef(I->getOperand(0).getReg()) will simply be MI1 after
emitCxxCaptureStmts().

This isn't quite NFC because I've also fixed a bug that I'm surprised we
haven't encountered yet. It now checks there are at least the expected
number of operands before accessing them with getOperand().

Depends on D30531

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Mar 2 2017, 6:55 AM
rovka edited edge metadata.Mar 14 2017, 6:20 AM

I think this looks ok overall, just a few nits.

There's a typo in the summary (I assume the first "capturing each" is superfluous):

It works by capturing each checking the 'shape' of the match and capturing
each matched instruction to a local variable

test/TableGen/GlobalISelEmitter.td
46 ↗(On Diff #90332)

I would check for the "return true" as well.

utils/TableGen/GlobalISelEmitter.cpp
577 ↗(On Diff #90332)

Are you planning to add more checks here? If not, I would suggest naming this getOperand and renaming getOperand to getOperandOrNull (otherwise it's a bit unclear what it's checking without looking at the code).

595 ↗(On Diff #90332)

This isn't capturing anything, it's just checking that we can capture. I would update the comment to reflect that, otherwise it looks a bit out-of-sync.
Ideally, we'd have a more expressive name for emitCxxCaptureStmts, but emitCxxCheckAndCaptureStmts is a bit long and I can't think of a better one, so just fixing the comments should do.

900 ↗(On Diff #90332)

Why is this a lambda? If it's just for readability, wouldn't it be easier to just generate comments along the lines of "Code for <blah> rule" ?

903 ↗(On Diff #90332)

I would move this comment to the emitCxxCaptureStmts declaration.

dsanders added inline comments.Mar 15 2017, 9:47 AM
test/TableGen/GlobalISelEmitter.td
46 ↗(On Diff #90332)

Ok

utils/TableGen/GlobalISelEmitter.cpp
577 ↗(On Diff #90332)

No, the 'Checked' is just to distinguish between the case that's allowed to fail and the one that isn't. I'll rename it.

What do you think about getOptionalOperand (returning an Optional<>) for the one that's allowed to fail? I think that might fit better with the support for OperandWithDefaultOps I'm looking into at the moment.

595 ↗(On Diff #90332)

Good point. It's part of the code to capture the locals but the name is misleading in this case. I can't think of any good alternatives either so I'll update the comments.

900 ↗(On Diff #90332)

The main thing is that it allows me to declare MachineOperands in the middle of the checks. There's no default constructor so I can't declare them and assign to them later and I didn't want to add redundant MachineOperand::CreatePlaceholder() calls.. The alternative was to use additional scopes and 'if (!condition) goto ruleN;' but I thought that looked worse.

903 ↗(On Diff #90332)

Ok

dsanders edited the summary of this revision. (Show Details)Mar 15 2017, 4:34 PM
rovka accepted this revision.Mar 16 2017, 9:14 AM

LGTM.

utils/TableGen/GlobalISelEmitter.cpp
577 ↗(On Diff #90332)

Ok, sounds good.

This revision is now accepted and ready to land.Mar 16 2017, 9:14 AM
dsanders updated this revision to Diff 92031.Mar 16 2017, 11:27 AM

Account for review comments

dsanders marked 11 inline comments as done.Mar 16 2017, 11:29 AM
dsanders closed this revision.Mar 20 2017, 8:32 AM
This revision was automatically updated to reflect the committed changes.