Page MenuHomePhabricator

WIP: [MachineOperand][MIR] Add isRenamable to MachineOperand.
ClosedPublic

Authored by gberry on Oct 27 2017, 9:58 PM.

Details

Summary

Add isRenamable() predicate to MachineOperand. This predicate can be
used by machine passes after register allocation to determine whether it
is safe to rename a given register operand. Register operands that
aren't marked as renamable may be required to be assigned their current
register to satisfy constraints that are not captured by the machine
IR (e.g. ABI or ISA constraints).

Diff Detail

Repository
rL LLVM

Event Timeline

gberry created this revision.Oct 27 2017, 9:58 PM
fhahn added a subscriber: fhahn.Oct 30 2017, 6:28 AM
gberry updated this revision to Diff 121965.Nov 7 2017, 12:58 PM

Update lit tests.

gberry added a comment.Nov 7 2017, 1:01 PM

Ping.
This change should be in pretty good shape now, but the MI interface changes could use a good review.
I had to update quite a few MIR tests to add the 'norename' flag. A lot of these could probably be fixed by getting the relevant passes to set the norename flag more precisely, but I figured it would be better to do that in separate changes.

MatzeB added inline comments.Nov 15 2017, 10:28 AM
include/llvm/CodeGen/MachineOperand.h
88–89 ↗(On Diff #121965)

Written like this it'll probably be part of the IsDef comment in doxygen. Maybe just append a "Only valid for MO_Register" to each of the four fields instead?

103 ↗(On Diff #121965)

The last sentence is not true anymore.

106 ↗(On Diff #121965)

One interesting discussion here is how "safe" things really are when this bit is set :)
I think AMDGPU for example has some extra constraints on physregs that the register allocator isn't aware of, they fix things in a separate pass after allocation AFAIK and after that you better not rename anymore. I think we currently set the ExtraSrcRegAllocReq and ExtraDefRegAllocReq on those machine instructions to indicate that. Then again we the infrastructure of this of this patch in place we could require them to clear the renamable flag on all affected operands.

107–109 ↗(On Diff #121965)

Should we state that this bit only affects physical registers? That way we don't need to set this bit everywhere a vreg is used. And I can't imagine any case where we have a non-renamable vreg or put another way: I do not want to add code dealing with non-renamable vregs.

310–311 ↗(On Diff #121965)

I assume changing this to assert(isReg() && IsDef()) breaks too much code?

368–372 ↗(On Diff #121965)

See my comment in setReg(), I think we shouldn't have this method.

419 ↗(On Diff #121965)

Maybe add assert(TargetRegisterInfo::isPhysicalRegister(getReg() && "Renamable flag may only be set for physregs");

lib/CodeGen/MIRParser/MIParser.cpp
1221 ↗(On Diff #121965)
  • "operands" instead of "MIR regs"?
lib/CodeGen/MIRParser/MIRParser.cpp
420–428 ↗(On Diff #121965)

I'm not a fan of the parsing changing depending on a function property. I think the right thing to do is to encode "renamable" and not "norename" after all.

If you are worried about mir output becoming noisy/hard to read after regalloc with "renamable" flags everywhere, maybe we can shorten "renamable" to some character like ~?

lib/CodeGen/MachineInstr.cpp
90–91 ↗(On Diff #121965)

I find it surprising that setReg() affects a flag as well. I'd rather just modify the regallocators to set the flag explicitely where possible.

lib/CodeGen/MachineVerifier.cpp
365–366 ↗(On Diff #121965)

NoVRegs should mean exactly that: We are guaranteed that no vregs are used. It does not mean: We are in a pass after register allocation.

1209–1214 ↗(On Diff #121965)

As stated above I would go for Renamable only ever being set on physregs. That way the rules don't need to be conditional based on "regsAllocated".

gberry added a comment.Dec 4 2017, 2:55 PM

@MatzeB I have a new version that is much simpler that I believe addresses all of your points. I got rid of the context-dependent printing/parsing of the renamable flag (and changed it from 'norename') which ended up reducing the amount of lit test churn. I also restricted it to physical registers and tried to enforce the hasExtraRegAllocReq constraints, though that may not be complete.

include/llvm/CodeGen/MachineOperand.h
310–311 ↗(On Diff #121965)

Yeah, I tried adding it and gave up after fixing it in about 5 different passes.

gberry updated this revision to Diff 125424.Dec 4 2017, 2:56 PM

New version based on feedback from Matthias

MatzeB accepted this revision.Dec 4 2017, 4:03 PM

Thanks, conditional LGTM.

My only remaining issue here is addOperand() changing operands. If you can easily fix this just commit without further review, otherwise at least write a comment on why you need it.

include/llvm/CodeGen/MachineOperand.h
106 ↗(On Diff #121965)

I would still vote for a weaker language here by removing the word "safely".

lib/CodeGen/MachineInstr.cpp
263–268 ↗(On Diff #125424)

Is this a good idea? Intuitively I would not expect an addOperand() function to change the operand. Admittedly someone already broke that rule with the setIsEarlyClobber() above but unless all other solutions are really bad, I'd still prefer not to have this here.

This revision is now accepted and ready to land.Dec 4 2017, 4:03 PM
MatzeB added a comment.EditedDec 4 2017, 4:05 PM

It would also be nice to pick a suitable test in test/CodeGen/MIR/X86 and add one line to ensure .mir prints/parses the flag correctly.

gberry marked an inline comment as done.Dec 5 2017, 10:51 AM

I made the comment less strong and added a test case. Let me know what you think about the addOperand issue.

lib/CodeGen/MachineInstr.cpp
263–268 ↗(On Diff #125424)

Without this every bit of code that is builds an instruction whose opcode hasExtraRegAllocReq would need to remember to clear the appropriate renamable bit. It seems to me that the hasExtraRegAllocReq property is less useful that way. I suppose we could untie these two properties and ask that users check both the renamable bit and the hasExtraRegAllocReq (with a helper function), in which case the verification that the two properties are consistent goes away.

gberry updated this revision to Diff 125575.Dec 5 2017, 10:51 AM

Made comment less strong and added MIR test case

MatzeB added inline comments.Dec 5 2017, 12:18 PM
lib/CodeGen/MachineInstr.cpp
263–268 ↗(On Diff #125424)

But unless I miss something, the default is not-renamable anyway so all the random code out there doesn't need to be changed. The only places that set the renamable bit are the regallocators and they already use MO.setIsRenamableIfNoExtraRegAllocReq() in your commit. So what other code is there that would set the renamable bit even though it shouldn't?

gberry added inline comments.Dec 5 2017, 1:12 PM
lib/CodeGen/MachineInstr.cpp
263–268 ↗(On Diff #125424)

One example that occurs in the lit tests is code that is constructing a hasExtraReqAllocReq machine instruction after register allocation and copying over operands from an existing instruction. These particular operands are renamable since they were originally virtual regs. Once they are added to the new instruction they remain renamable, but are operands of a hasExtraRegAllocReq opcode so they fail verification.

MatzeB added inline comments.Dec 5 2017, 1:17 PM
lib/CodeGen/MachineInstr.cpp
263–268 ↗(On Diff #125424)

I assume that will be mostly (or all) AMDGPU code, is adjusting this code so hard that it justifies extra logic in a core function like addOperand()? At least we have the asserts in place and won't silently miscompile...

gberry updated this revision to Diff 125994.Dec 7 2017, 10:47 AM

New version that doesn't force renamable flag to false for hasExtraRegAlloc opcodes in addOperand

gberry added inline comments.Dec 7 2017, 10:50 AM
lib/CodeGen/MachineInstr.cpp
263–268 ↗(On Diff #125424)

I added a new version that removes this change to addOperand and fixes the lit test failures (which only showed up for ARM). I'm a bit concerned that this approach will break other targets/tests, but I suppose we can always revisit this later if it is a problem.

This revision was automatically updated to reflect the committed changes.