Page MenuHomePhabricator

[GlobalISel] Fix constrainOperandRegClass to insert copies in the right position for reg definitions
ClosedPublic

Authored by kariddi on Apr 25 2019, 3:56 PM.

Details

Summary

There is a pretty big bug in constrainOperandRegClass when called to constrain a definition and it requires to create a new virtual register and add a corrective COPY.

When the constraining happens on a use the COPY needs to be before the instruction that contains the MachineOperand, but if we are constraining a definition it actually needs to be added after the instruction.
In addition, the COPY needs to have its operands flipped (in the use case we are copying from the old unconstrained register to the new constrained register, while in the definition case we are copying from the new constrained register that the instruction defines to the old unconstrained register).

This apparently never triggered because no globalisel backends today seem to need the constraining of a definition, but it does on an out-of-tree backend.

This patch fixes that and in addition I moved the logic to insert the copy from constrainRegToClass() to constrainOperandRegClass() as it really belonged to the second one (the first seem to be working exclusively with pure register numbers without any concept of which instruction uses it, while the second it is actually called to constrain a very specific operand and so there we already have all the information about insertion points and if it is a definition or a use of such a register)

Diff Detail

Repository
rL LLVM

Event Timeline

kariddi created this revision.Apr 25 2019, 3:56 PM

LGTM. Thanks for fixing this.

This revision is now accepted and ready to land.Apr 25 2019, 7:20 PM
kariddi closed this revision.Apr 26 2019, 12:20 AM

Closed by r359282 (forgot to add differential revision to commit message)