Page MenuHomePhabricator

[FastISel][X86] Constrain register operand class in foldOperandImpl
Needs ReviewPublic

Authored by loladiro on Nov 13 2014, 10:01 PM.

Details

Reviewers
pete
ributzka
Summary

instead of cleaning up after. This is made possible by a previous
commit (D10174) that threaded InsertPt through foldOperandImpl.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 16197.Nov 13 2014, 10:01 PM
loladiro retitled this revision from to [FastISel][X86] Assign the correct register class to folded operations .
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro added a reviewer: ributzka.
loladiro set the repository for this revision to rL LLVM.
loladiro updated this object.
loladiro updated this object.
loladiro added a subscriber: Unknown Object (MLST).
ributzka edited edge metadata.Nov 14 2014, 10:41 AM

I think this should be done inside 'foldMemoryOperandImpl'. We shouldn't have to cleanup afterwards.

'constrainRegClass' may actually fail, so you need to check the result of this method if you use it .

lib/Target/X86/X86FastISel.cpp
3348

I don't think this is correct. The index "i" must line up with the MO index in the instruction and not with AddrOps.

3350

MO.isReg() is shorter :)

3353

If you use TargetRegisterInfo::isVirtualRegister instead, you can even skip the MO.isReg() check.

3355

This is not guaranteed to succeed - you need to check the result.

See FastISel::constrainOperandRegClass for more details.

I looked at implementing this in foldMemoryOperandImpl, but it seems like right now it can only return one instruction and does not insert it, which doesn't work if we want to insert the COPY for constraining the register class.

I did see a FIXME that says

// FIXME: change foldMemoryOperandImpl semantics to also insert NewMI.

so maybe I should just try that?

loladiro updated this revision to Diff 16265.Nov 15 2014, 12:31 AM
loladiro edited edge metadata.

Ok, have a look at this and let me know what you think. It threads through the InsertPt and then sets the correct register class in foldMemoryOperandImpl. If this approach generally looks fine, I'll split out threading through the InsertPt into a separate commit (since it touches other backends).

loladiro added a subscriber: pete.Jun 1 2015, 12:35 PM

@pete looks like you fixed this one as well (though this time in rL236644). Maybe I'll just wait for somebody else to fix it next time ;). Would you mind taking a look here and seeing if this is still interesting?

pete added a comment.Jun 1 2015, 1:14 PM

@pete looks like you fixed this one as well (though this time in rL236644). Maybe I'll just wait for somebody else to fix it next time ;). Would you mind taking a look here and seeing if this is still interesting?

Yep, looks like my fix handled this one too. I don't think this patch will handle more cases than mine. If you have a strong preference for your solution over mine, then i'm happy to see the code changed though. There's no perfect way to fix this bug so i'm happy to see the best possible solution committed.

Well, as you can see the first version of this patch was actually exactly the same as yours and @ributzka asked me to refactor it. It's been a while since I looked at this code, but it seems like I can factor out adding the InsertPt, which is the bulk of this change as an NFC commit and then only have the actual bugfix here. Does that sounds good?

pete added a comment.Jun 1 2015, 3:11 PM

Yeah, sounds good to me. Thanks!

loladiro updated this revision to Diff 26942.Jun 1 2015, 5:35 PM
loladiro retitled this revision from [FastISel][X86] Assign the correct register class to folded operations to [FastISel][X86] Constrain register operand class in foldOperandImpl .
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro added a reviewer: pete.
loladiro set the repository for this revision to rL LLVM.

Split out the NFC changes as D10174. This is what remains.

Ok, the other changes have gone in. @pete does this look ok to you?

pete edited edge metadata.Jun 8 2015, 7:14 PM

Sorry, Phab is acting up, so will use email.

This was in the original, but as your moving the code anyway, can you invert this to remove indentation, i.e.,

+ if (TargetRegisterInfo::isVirtualRegister(Op)) {

}
return Op

to

+ if (!TargetRegisterInfo::isVirtualRegister(Op))

return Op;

Similarly, can you make this a foreach loop:

for (unsigned i = 0; i != NumAddrOps; ++i) {

to

for (MachineOperand MO : MOs) {

Thanks for doing all this!

Cheers,
Pete

The i in the for loop is actually required as it's passed into constrainOperandRegClass. For the other one, the success case of constrainRegClass, does also lead to the second return statement, so I think it's cleaner this way than

unsigned MachineRegisterInfo::constrainOperandRegClass(
    MachineFunction &MF, MachineBasicBlock::iterator InsertPt, DebugLoc DL,
    const MCInstrDesc &II, unsigned Op, unsigned OpNum,
    const TargetInstrInfo &TII, const TargetRegisterInfo &TRI) {
  if (!TargetRegisterInfo::isVirtualRegister(Op))
    return Op;
  const TargetRegisterClass *RegClass = TII.getRegClass(II, OpNum, &TRI, MF);
  if (constrainRegClass(Op, RegClass))
    return Op;
  // If it's not legal to COPY between the register classes, something
  // has gone very wrong before we got here.
  unsigned NewOp = createVirtualRegister(RegClass);
  BuildMI(*InsertPt->getParent(), InsertPt, DL, TII.get(TargetOpcode::COPY),
          NewOp)
      .addReg(Op);
  return NewOp;
}

but I have no strong opinion.