[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

[Jakob Stoklund Olesen]

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/1a432969/attachment.html>