Page MenuHomePhabricator

[globalisel][tablegen] Support zero-instruction emission.
ClosedPublic

Authored by dsanders on Jul 31 2017, 5:25 AM.

Details

Summary

Support the case where an operand of a pattern is also the whole of the
result pattern. In this case the original result and all its uses must be
replaced by the operand. However, register class restrictions can require
a COPY. This patch handles both cases by always emitting the copy and
leaving it for the register allocator to optimize.

Depends on D35833

Event Timeline

dsanders created this revision.Jul 31 2017, 5:25 AM

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

dsanders updated this revision to Diff 108909.Jul 31 2017, 5:59 AM

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

It looks like it disappeared during a rebase and is already committed. That
being the case, I've just removed the C++ implementation of bitcast and
int_to_ptr that are covered by imported rules.

dsanders updated this revision to Diff 109074.Aug 1 2017, 3:56 AM

Fix the previous update so that it contains both the original patch and the correction.

rovka edited edge metadata.Aug 2 2017, 5:51 AM

Hi Daniel,

Can you point me to the test that you've already committed? Was it an AArch64 inst select test? If so, I think you should also add a test in GlobalISelEmitter.td (I see one for a bitconvert -> COPY_TO_REGCLASS, but none checking mutations to COPY).

Thanks,
Diana

lib/Target/AArch64/AArch64InstructionSelector.cpp
1169 ↗(On Diff #109074)

Can you point out those rules? The comments in SelectionDAGCompat say that G_INTTOPTR has no SelectionDAG equivalent, so I'm not sure where to look.

1171 ↗(On Diff #109074)

Doesn't the destination always have to be a pointer and the source a scalar?

utils/TableGen/GlobalISelEmitter.cpp
2293

Would it make sense to move these 2 checks higher up, as a quick exit? (So we don't createAndImport the Src only to discover that Dst wasn't an instruction in the first place).

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

Hi Daniel,

Can you point me to the test that you've already committed? Was it an AArch64 inst select test? If so, I think you should also add a test in GlobalISelEmitter.td (I see one for a bitconvert -> COPY_TO_REGCLASS, but none checking mutations to COPY).

Thanks,
Diana

It was test/CodeGen/AArch64/GlobalISel/select-bitcast.mir and the imported rules that cover it are the bitconvert -> COPY_TO_REGCLASS rules which end up as COPY nodes with a constraint on the result. I considered treating COPY_TO_REGCLASS as a simple COPY but I decided against it in case it mattered on some targets.

lib/Target/AArch64/AArch64InstructionSelector.cpp
1169 ↗(On Diff #109074)

I think I'm being confusing here. As you point out below, G_INTTOPTR always has a pointer result and as a result there are no rules that need to be imported to cover other cases.

1171 ↗(On Diff #109074)

Good point. The only cases that we don't handle here are those that can't occur. I'll remove the if-statement.

1173 ↗(On Diff #109074)

I've just noticed I'm missing a return false before this line.

1181 ↗(On Diff #109074)

I've just noticed I'm missing a return false before this line.

utils/TableGen/GlobalISelEmitter.cpp
2293

We could do that. The only reason it comes after the matcher is to keep a boundary between the code that handles the matcher and the code that handles the renderer. The two were tangled together in the early versions of GlobalISelEmitter.cpp.

dsanders updated this revision to Diff 110196.Aug 8 2017, 7:35 AM
dsanders marked 6 inline comments as done.

Fix the issues with the changes to the C++ implementation of G_INTTOPTR and G_BITCAST.

dsanders added inline comments.Aug 8 2017, 7:36 AM
utils/TableGen/GlobalISelEmitter.cpp
2293

I just went to do this and I don't think it can be moved up without significant changes. The Dst->getOperator() on line 2285 requires that Dst->isLeaf() is false so it must either come after the if-statement on line 2255 or be wrapped in a if (!Dst->isLeaf()) {}. If we choose to move the if-statement on line 2255 up as well then we're creating the renderer before the matcher which will break getOperand(StringRef) because the variables (defineInsnVar()) wont be defined at that point. If we chose to wrap it in an if (!Dst->isLeaf()) and move it past line 2255's if-statement then we have a use-beyond-scope issue to fix and fixing that requires allowing DstOp to be null and rewriting DstI to be a (possibly-null) pointer which will impact the createAndImportInstructionRenderer() function and those it calls.

rovka added a comment.Aug 11 2017, 1:53 AM

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

LGTM if you add that test.

utils/TableGen/GlobalISelEmitter.cpp
2260

Typo: its

2293

Ok, it's fine as it is then.

This revision was automatically updated to reflect the committed changes.
dsanders reopened this revision.Aug 11 2017, 12:20 PM

Two windows bots failed select-inc.mir which should have been unaffected by the change. Investigating

This revision was automatically updated to reflect the committed changes.
dsanders reopened this revision.Aug 15 2017, 8:14 AM
This revision was automatically updated to reflect the committed changes.