[llvm-commits] [llvm] r113670 - in /llvm/trunk: include/llvm/Target/TargetInstrInfo.h lib/CodeGen/PeepholeOptimizer.cpp lib/Target/ARM/ARMBaseInstrInfo.cpp lib/Target/ARM/ARMBaseInstrInfo.h

[Gabor Greif]

On 9/13/10, Bill Wendling <wendling at apple.com> wrote:
> On Sep 13, 2010, at 11:04 AM, Gabor Greif wrote:
>
>> On 12 Sep., 02:04, Bill Wendling <wendl... at apple.com> wrote:
>>> I think the TST instruction can take a compare value other than 0. The
>>> CMP instructions take 0, right?
>> TST instructions have 2 operands and the second is a mask. It is a
>> hybrid of "and" and "cmp" against 0.

I should have worded more carefully here. I mean the TSTri
opcode. The corresponding MachineInstruction has 2 operands:
 - the operand register 1
 - the mask immediate

>
> The third operand is a "shift" when using a register-register test
> (according to my documentation). But we covered that in a previous email.

This would be TSTrs. I have no ambition to peephole optimize those (yet?).

>
>>> Below is what I have in my tree right now. It's just the skeleton of how
>>> I'd like to handle this (it does nothing right now). I like it because of
>>> how it separates out different optimization potentialities for each type
>>> of instruction, but it may be overkill...
>>
>> Yes, I think it is a bit of overkill. Logically TST is a compare
>> instruction. In my last patch I successfully handle both with one
>> sweep. If you think my approach is sufficient and works well on your
>> testcases, I gladly check it in. Then I would like to establish
>> something like
>>
>> dyn_cast<ARMri<ARM::ANDri> >
>> queries (with additional intelligence) for checking opcodes and
>> getting operands. Would simplify my code a great amount.
>>
> Here's your patch from another email. My comments are in-line:
>
> gabor at google8:~/llvm-build$ svn diff /home/gabor/llvm/lib/Target/ARM/
> ARMBaseInstrInfo.cpp
> Index: /home/gabor/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
> ===================================================================
> --- /home/gabor/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
> (revision 113747)
> +++ /home/gabor/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
> (working copy)
> @@ -1372,6 +1372,19 @@
>     SrcReg = MI->getOperand(0).getReg();
>     CmpValue = MI->getOperand(1).getImm();
>     return true;
> +  case ARM::TSTri: {
> +      if (MI->getParent()->begin() ==
> MachineBasicBlock::const_iterator(MI))
> +        return false;
> +      const MachineInstr *AND = llvm::prior(MI);
>
> The AND doesn't have to be the immediately prior instruction. I could happen
> further up in the BB just as long as there isn't an intervening instruction
> which sets CPSR.

Yes, but the instruction selector tends to create sequences like this:

  rN = ...
  rM = and rN, #mask
  ... ; intervening instructions
  tst rN, #mask

Note that both "and" and "tst" consume rN! But if we want to change
the form of "and" to "andS" we have to find it. My first guess is to
look immediately before the "tst". I may relax that (to, say 5
predecessors), but do not want to risk a quadratic algorithm.

>
> +      if (AND->getOpcode() != ARM::ANDri)
> +        return false;
> +      if (MI->getOperand(0).getReg() == AND->getOperand(1).getReg() && //
> FIXME: or == AND
> +          MI->getOperand(1).getImm() == AND->getOperand(2).getImm()) {//
> FIXME: subset
> +        SrcReg = AND->getOperand(0).getReg();
> +        CmpValue = 0;
>
> CmpValue might not be 0 here.

Whatever the mask is, the comparison is always against zero for the
TSTri opcode. I hope I am not terribly mistaken with this statement
:-)

>
> +        return true;
> +      }
> +    }
>
> There's no need for this to be as complex here. The logic should be pretty
> much identical to the CMP cases. In fact, I think it is identical when you
> have a TST of an immediate. This logic should reside in

I only attempt to cover the TSTri case. I do *not* think it is just as
easy as "CMPri", because of the search for a suitable ANDri
instruction that feeds from the same input
and uses the same (super-) mask. Only in this case the "andS" will be
equivalent.

> ARMBaseInstrInfo::OptimizeCompareInstr. Once you do that, the tests of
> SrcReg & CmpValue become trivial.
>
> I need to comment this method better. It's supposed to return the source
> register and compare values of the comparison instruction.

Yes, I think I got this.

>
>   }
>
>   return false;
> @@ -1421,6 +1434,7 @@
>   switch (MI->getOpcode()) {
>   default: break;
>   case ARM::ADDri:
> +  case ARM::ANDri:
>
> Also do t2ANDri.

Sure, adding it.

>
>   case ARM::SUBri:
>   case ARM::t2ADDri:
>   case ARM::t2SUBri:
>
> To do the optimization you suggest should be almost as easy as simply adding
> the TST* cases to the switch statements and removing the "if (CmpValue != 0)
> return false;" from OptimizeCompareInstr. :-)

Are you talking about TSTrr and TSTrs here?

Cheers,

   Gabor

>
> -bw
>
>