Page MenuHomePhabricator

[SelectionDAG] Provide adequate register class for RegisterSDNode
ClosedPublic

Authored by smaksimovic on Jul 18 2017, 7:34 AM.

Details

Summary

When adding operands to machine instructions in case of RegisterSDNodes,
generate a COPY node in case the register class does not match the one in the instruction definition.

Diff Detail

Event Timeline

smaksimovic created this revision.Jul 18 2017, 7:34 AM
sdardis edited edge metadata.Jul 25 2017, 6:06 AM

I think the approach here is incorrect, as this patch is solving a very specific case when the bug is more general.

During the instruction emission of SDNodes to MachineInstrs, InstrEmitter::AddOperand() fails to account
for handling register operands whose register class differs from the instruction definition.

Normally the initial register class for an incoming register operand is chosen based on the value type it
holds, which in this case is GPR32. The problematic SW16_MM however takes a GPRMM16Zero register
operand.

Can you look at modifying MachineInstrs, InstrEmitter::AddOperand() to use AddRegisterOperand in the
case of RegisterSDNodes?

Thanks,
Simon

Compare the register classes one would get for the machine instruction operand Op as opposed to the register class we would get based on the MCInstrDesc.
If those two would differ, we would emit an additional COPY node before adding the register which now has the desired register class.
Tried using the AddRegisterOperand() directly, but couldn't do so since getVR called inside cannot look up the SDValue Op inside the map -- that value happens to be a COPY node as well for which .isMachineOpcode() returns false.
As far as I can tell, additions to that map are being done in functions called from InstrEmitter::EmitMachineNode() which I assume deals with machine nodes only.

sdardis requested changes to this revision.Dec 12 2017, 6:51 AM

I believe the entire hunk for handling RegisterSDNodes should be sunk into AddRegisterOperand as your change mimics portions of the logic there.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp
408 ↗(On Diff #116164)

Space after the if, and the line is overly long.

This revision now requires changes to proceed.Dec 12 2017, 6:51 AM
smaksimovic retitled this revision from [mips] Insert a COPY node for SW16_MM to Provide adequate register class for RegisterSDNode.
smaksimovic edited the summary of this revision. (Show Details)

Please update the title with [SelectionDAG] tag and description to reflect that this is now a selection dag patch. Also, can you restore the code to where it previously was, I was incorrect about that, as it'd just make the code harder to follow in terms of handling virtual and physical registers in the same function.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp
319–321 ↗(On Diff #127351)

This comment should go above:

bool Imp = II && (IIOpNum >= II->getNumOperands() && !II->isVariadic());
330 ↗(On Diff #127351)

Formatting, space after the if.

smaksimovic retitled this revision from Provide adequate register class for RegisterSDNode to [SelectionDAG] Provide adequate register class for RegisterSDNode.
smaksimovic edited the summary of this revision. (Show Details)
sdardis added a subscriber: jvesely.Feb 2 2018, 4:38 AM

It appears this also fixes a machine verifier failure in test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll, but I am unsure on the correctness of the changed assembly output. +CC'ing Jan Vesely who provided the test.

It appears this also fixes a machine verifier failure in test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll, but I am unsure on the correctness of the changed assembly output. +CC'ing Jan Vesely who provided the test.

It'd help to see the diff. The test checks that certain registers are reserved and not allocated when thread IDs are used by the shader.
TIDIG.{x,y,z} is in T0.{x,y,z}, TGID.{x,y,z} is in T1.{x,y,z}.

It looks like this patch is OK other than introducing an extra MOV, I haven't looked into why that is.

Attached.

Attached.

These look good. An additional MOV is inserted since MEMRAT_CACHELESS STORE_RAW includes constraint of using _X subreg for 32bit stores.
When you edit the test please make sure to test both the MOV from the correct id register and store of the right data.

thank you.

Update test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll

Update test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll

the r600 test LGTM

sdardis accepted this revision.Feb 9 2018, 1:46 AM

LGTM.

test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll
1 ↗(On Diff #133193)

Add -verify-machineinstrs here to ensure we don't regress.

This revision is now accepted and ready to land.Feb 9 2018, 1:46 AM