[llvm-commits] [llvm] r157831 - in /llvm/trunk: lib/Target/X86/X86InstrArithmetic.td lib/Target/X86/X86InstrInfo.cpp lib/Target/X86/X86InstrInfo.h test/CodeGen/X86/jump_sign.ll

[Manman Ren]

Thanks, Pete and Jakob.

I will try that. I copied this from ARM's implementation, where the register is CPSR.

Manman

On Jun 4, 2012, at 9:46 AM, Jakob Stoklund Olesen wrote:

> 
> On Jun 2, 2012, at 11:07 PM, Pete Cooper <pete.cooper at gmail.com> wrote:
> 
>>> 
>>> > +    for (unsigned IO = 0, EO = Instr.getNumOperands(); IO != EO; ++IO) {
>>> > +      const MachineOperand &MO = Instr.getOperand(IO);
>>> > +      if (MO.isRegMask() && MO.clobbersPhysReg(X86::EFLAGS))
>>> > +        return false;
>>> > +      if (!MO.isReg()) continue;
>>> > +
>>> > +      // This instruction modifies or uses EFLAGS before we find a SUB.
>>> > +      // We can't do this transformation.
>>> > +      if (MO.getReg() == X86::EFLAGS)
>>> > +        return false;
>>> > +    }
>>> 
>>> Please hoist this loop into a helper function. Ideally on MachineInstr so others can use it. It seems generically useful.
>>>   
>>> 
>> 
>> Instead of hoisting this loop, please take a look at MI->modifiesRegister and MI->readsRegister.  I think calling both should match the functionality here.
> 
> Yep.
> 
>>  I think MI->readsWritesVirtualRegister matches it even better, but that function specifically mentions virtual registers in its name even though from its implementation I think it would be safe here on the EFLAGS physical register.
> 
> No, we ought to have an assertion catching abuses like that.
> 
> /jakob
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120604/aaa75b28/attachment.html>