[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

[Chandler Carruth]

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.

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.

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