[MachineOperand][Target] MachineOperand::isRenamable semantics changes
ClosedPublic

Authored by gberry on Feb 7 2018, 1:37 PM.

Details

Summary

Add a target option AllowRegisterRenaming that is used to opt in to
post-register-allocation renaming of registers. This is set to 0 by
default, which causes the hasExtraSrcRegAllocReq/hasExtraDstRegAllocReq
fields of all opcodes to be set to 1, causing
MachineOperand::isRenamable to always return false.

Set the AllowRegisterRenaming flag to 1 for all in-tree targets that
have lit tests that were effected by enabling COPY forwarding in
MachineCopyPropagation (AArch64, AMDGPU, ARM, Hexagon, Mips, PowerPC,
RISCV, Sparc, SystemZ and X86).

Add some more comments describing the semantics of the
MachineOperand::isRenamable function and how it is set and maintained.

Change isRenamable to check the operand's opcode
hasExtraSrcRegAllocReq/hasExtraDstRegAllocReq bit directly instead of
relying on it being consistently reflected in the IsRenamable bit
setting.

Clear the IsRenamable bit when changing an operand's register value.

Remove target code that was clearing the IsRenamable bit when changing
registers/opcodes now that this is done conservatively by default.

Change setting of hasExtraSrcRegAllocReq in AMDGPU target to be done in
one place covering all opcodes that have constant pipe read limit
restrictions.

Diff Detail

Repository
rL LLVM
gberry created this revision.Feb 7 2018, 1:37 PM
escha added a comment.Feb 8 2018, 3:52 PM

This looks like it might fix the problem we've been having, but i'm still extremely nervous about the overall concept. this feels like a fantastically large amount of machinery for such a small optimization, including this dubiously defined "isRenamable" flag that applies to operand registers even though renamability seems to be more of a property of the instruction.

Furthermore, the fact that vregs are always renamable means it's impossible to pass down the "renamability" of a register from before register allocation: we couldn't even use this feature if we wanted to.

I'm not going to hold it up, but all of this feels like a very roundabout approach to me.

Hi Geoff,

I believe this patch is going in the right direction of allowing targets to safely bail out from the optimization.
Now, like Fiona said, the constraints we want to express are either tied to instructions (encoding constraint not modeled in TableGen) or operands (ABI) but we only carry the information around in the machine operands. This is a problem, because each time we transfer operands, we need to make sure the operands inherit what the instructions want.

The setDesc part of the patch goes in that direction, but is not enough.
For instance, in expand post ra pseudo, we transfer the implicit operands of a copy to a lowered version of that same copy. By doing that we don't reset the renamable flags on the operand and potentially we would end up with renamable operands on a hasExtraXXXRegAllocReq instruction. (The setDesc fix wouldn't be enough since the transfer happens after the creation.)

One way we could fix that is by making isRenamable check both the flag on the operand and the hasExtraXXX flag of its parent (if any).
The plus side is that we wouldn't need to change setDesc.

What do you think?

Cheers,
-Quentin

Checking hasExtraRegAllocReg in isRenamable sounds reasonable to me (though it may require the interface to be changed slightly since we'll need the opcode). An alternative that is closer to what I initially proposed would be to have setReg and addOperand both clear the isRenamable flag. This way the only operands that would be renamable are those that have been untouched since they were marked renamable by the register allocator. This would also allow us to get rid of the extra code that I added to clear the renamable flag in some post-RA target code. Targets would be able to opt in to preserving the renamable flag by correctly preserving the renamable flag in post-RA transformed code, but by default the flag would be set conservatively. I'm going to try this approach out to see what it looks like and what kind of impact it has unless someone has a reason they think it won't work.

(though it may require the interface to be changed slightly since we'll need the opcode)

Shouldn't we just follow the getParent link on the operand?
If this is nullptr, we don't have to check it.

FYI, I've revert the original commit in r325421.

gberry updated this revision to Diff 135311.Feb 21 2018, 12:31 PM

New version based on review feedback.

gberry retitled this revision from [MachineOperand][Target] Add target option to disable setting MachineOperand::isRenamable to [MachineOperand][Target] MachineOperand::isRenamable semantics changes.Feb 21 2018, 12:33 PM
gberry edited the summary of this revision. (Show Details)
gberry added a reviewer: arsenm.

@arsenm Could you take a look at the AMDGPU change?

qcolombet accepted this revision.Feb 21 2018, 1:03 PM

Hi Geoff,

LGTM.

Could you check if you could add a test case, for instance running expand post RA pseudo?
You would need a target that expands copies into something with ExtraXXXRegAlloc and I don't know if that exists.

What I have in mind is something along the lines:
renamable dst = COPY renamable src

> expand post RA pseudo

dst = TargetInstr src

(The check would be that dst and src don't have renamable after the expansion.)

Cheers,
-Quentin

This revision is now accepted and ready to land.Feb 21 2018, 1:03 PM
gberry updated this revision to Diff 135468.Feb 22 2018, 10:45 AM
gberry edited the summary of this revision. (Show Details)

Add test/CodeGen/AMDGPU/postra-norename.mir as requested by Quentin

This revision was automatically updated to reflect the committed changes.