[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]

Updated the patches and submitted r159465 and r159470, corresponding to #1 and #2.
I will be modifying the patch for X86 (#3).

Thanks,
Manman

On Jun 29, 2012, at 1:41 AM, Chandler Carruth wrote:

> Sorry for the delays, and thanks for the ping. =] Had to switch this back into context.
> 
> On Wed, Jun 27, 2012 at 4:30 PM, Manman Ren <mren at apple.com> wrote:
> 
> I updated the patch, please review and provide feedback.
> 
> Update peephole optimization for X86 and ARM:
> 1> Interface: added SrcReg2 to analyzeCompare and optimizeCompare.
>                        AnalyzeCompare to analyzeCompare ...
> 
> 2> ARM: Clean up, no functional change.
> Replaced a loop with existing interfaces: modifiesRegister and readsRegister.
> Factored out code into inline functions and simplified the code.
> 
> I would submit these two as two separate patches first -- smaller, more focused patches make both review and triaging test failures much easier.
> 
> For #1, I can't really review the benefit of this change, perhaps because I don't know ARM... It seems plausible in the implementation, but you didn't update the comments for the methods to talk about the new parameter. I like to use full doxygen comments to help remind myself to document everything:
> 
> /// \brief This method does foo.
> ///
> /// Some details about how it does foo. Can be longer.
> /// \param[in] x An input parameter.
> /// \param[in,out] y An input and output parameter.
> /// \param[out] z An output parameter.
> 
> However, this can be very verbose. ;] Often, a really good parameter name/type obviates the need for this level of commenting, but 'SrcReg2' as an 'unsigned' doesn't really tell me much at all.
> 
> For #2, everything seems generally fine. One nit-pick:
> 
> -    SmallVector<MachineOperand*, 4> OperandsToUpdate;
> +    SmallVector<std::pair<MachineOperand*, ARMCC::CondCodes>, 4>
> +                                                               OperandsToUpdate;
> 
> I would just indent the second line here by 4 spaces instead of 2 spaces. Right-justifying things is really hard on most editors, and often quite hard for the reader...
> 
> I think with these tweaks these first two LGTM.
> 
> 
> 3> X86: added peephole optimization to remove cmp instruction
> It will optimize the following:
>   sub r1, r3
>   cmp r3, r1 or cmp r1, r3
>   bge L1
> TO
>   sub r1, r3
>   bge L1 or ble L1
> If the branch instruction can use flag from "sub", then we can eliminate
> the "cmp" instruction.
> 
> For this optimization, we need to first check the condition code of
> SET|CMOV|Jcc and then update the condition code.
> 
> This patch implemented 4 helper functions:
> getSwappedConditionForCMov
> getSwappedConditionForBranch
> getSwappedConditionForSET
> isRedundantFlagInstr
> 
> I can't think of a way to easily update the condition code of an instruction
> by adding flags in td file or data in MCInstrDesc, since the condition code
> is hard-coded in the opcode.
> 
> Ugh. I see the challenge here, it just results in a pretty impressive pile of switch code. However, I agree that there doesn't seem to be a better technique.
>  
> 
> From what I know, there are 3 pairs of equivalent falg instructions for ARM:
> CMP vs SUB, CMN vs ADD, TST vs AND
> 2 pairs for X86:
> CMP vs SUB, TST vs AND
> If there are more pairs, or there is a better way to implement isRedundantFlagInstr, please let me know.
> 
> Now I see what you're driving at, and why my comments were confusing. =]
> 
> You're trying to find instructions which set *exactly* equivalent flags for a set of inputs, modulo swapping the condition codes, and only these two make sense there.
Yes :)
> 
> What I was pointing out is that there are more narrow, but possibly interesting cases as well if you consider the specific flag being tested:
> 
> neg %rax
> ... <some non-flag-setting instructions> ...
> test %rax,%rax
> jz ...
> 
> Here, we're just testing for %rax to be zero, and neg actually sets CF if its operand is zero, so we could transform this to:
> 
> neg %rax
> ... <some non-flag-setting instructions> ...
> jc ...
> 
> Here is one I've actually seen in real code:
> 
> bsr %rax,%rbx
> ...
> test %rax,%rax
> cmovz 0,%rbx
> 
> Where the test can be deleted -- the flags are even the same.
For ARM, this is handled in ARM peephole, where "CMP %reg, 0" can be eliminated if the destination instruction of %reg can be used to update the flags.
I thought this is covered in X86 somehow, maybe in DAG combine.
If you happen to have a test case for X86, where a "CMP %reg, 0" can be removed, please let me know.
> 
> There are lots of equivalences similar to this that aren't true for all flags, but may be true for the specific flag in use. Clearly, any extension to cover these other patterns should be a follow-up patch, I just wanted to clarify what I was talking about.
> 
> 
> 
> For the actual patch part of #3, I think you skipped a few of my comments:
> 
> 
>> >
>> > +                     int CmpValue, const MachineRegisterInfo *MRI) const {
>> > +  MachineRegisterInfo::def_iterator DI = MRI->def_begin(SrcReg);
>> > +  if (llvm::next(DI) != MRI->def_end())
>> > +    // Only support one definition.
>> > +    return false;
>> > +
>> > +  MachineInstr *MI = &*DI;
>> > +  // Get ready to iterate backward from CmpInstr.
>> > +  MachineBasicBlock::iterator I = CmpInstr, E = MI,
>> > +                              B = CmpInstr->getParent()->begin();
>> 
>> Reverse iterators please. Iterating backwards is too hard to follow otherwise. Add the iterators if they're missing.
>>  
>> >
>> > +
>> > +  // Early exit if CmpInstr is at the beginning of the BB.
>> > +  if (I == B) return false;
>> 
>> Set up your loop variables below, where you're looping. You shouldn't need to do early exits etc, the loop simply won't iterate...
>> 
> 
> Specifically these two.
> 
> 
> Other comments:
> 
> +/// isRedundantFlagInstr - check whether the first instruction, whose only
> +/// purpose is to update flags, can be made redundant.
> +/// CMPrr can be made redundant by SUBrr if the operands are the same.
> +/// CMPri can be made redundant by SUBri if the operands are the same.
> +/// This function can be extended later on.
> +inline static bool isRedundantFlagInstr(MachineInstr *FlagI, unsigned SrcReg,
> +                                        unsigned SrcReg2, int ImmValue,
> +                                        MachineInstr *OI) {
> 
> Why is it necessary to pass in the registers and immediate values? That is, why not extract them from FlagI the way we do from OI? I see that the outer API does (and mentioned up above that I didn't quite understand it), and seeing this I'm a bit more confused. I'm fine with either an explanation in comments, or changing the interface if that makes sense -- I'm just trying to get the code to be more clear to a future reader, not claiming what the right interface is.
> 
> Also, is there a better name than OI?
> 
> +  for (MachineBasicBlock::iterator I = CmpInstr,
> +       E = CmpInstr->getParent()->end(); !IsSafe && ++I != E; ) {
> 
> Don't shadow the 'I' loop variable you already have here... This may get fixed by fixing the two comments above, but it needs to get fixed.
> 
> Also, it would be very nice to avoid incrementing inside the condition expression.... That's subtle and likely to be missed in the future.
> 
> Actually, you don't use IsSafe anywhere that I see. You 'break' out of the loop right after setting it to true, so its never even compared. Can you just kill this variable?
> 

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