[llvm] r180573 - This patch adds the X86FixupLEAs pass, which will reduce instruction

[Michael Liao]

Hi Preston

It seems to me that this fixup doesn't always do the right thing. Since
you change ADD/MOV to LEA, this shift AGI penalty from ADD/MOV to their
sink to their source to them. E.g. you will change

   1: MUL %ECX, %EDX
      ...
   2: ADD %EBX, %EDX
      ...
   3: MOV (%EDX), %EAX

to

   1: MUL %ECX, %EDX
      ...
   2: LEA (%EBX, %EDX), %EDX
      ...
   3: MOV (%EDX), %EAX

The AGI penalty is shifted from (2,3) to (1,2). It doesn't improve the
performance but increases the code size. Such fix-up should be reverted
if it just shift that penalty somewhere.

Yours
- Michael



On Thu, 2013-04-25 at 21:07 +0000, Gurd, Preston wrote:
> Hello Nadav,
> 
>  
> 
> Thanks, once again, for your attentiveness!
> 
>  
> 
> I had added the documentation just ahead of each function body, using
> X86PadShortFunction.cpp and X86VZeroUpper.cpp as models.
> 
>  
> 
> I will move the comments for the functions in X86FixupLEAs.cpp to
> precede the initial declarations.
> 
>  
> 
> Preston
> 
>  
> 
> From: Nadav Rotem [mailto:nrotem at apple.com] 
> Sent: Thursday, April 25, 2013 4:51 PM
> To: Gurd, Preston
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm] r180573 - This patch adds the X86FixupLEAs pass,
> which will reduce instruction
> 
> 
>  
> 
>         +
>         +    virtual const char *getPassName() const { return "X86
>         Atom LEA Fixup";}
>         +    void  seekLEAFixup(MachineOperand& p,
>         MachineBasicBlock::iterator& I,
>         +                      MachineFunction::iterator MFI);
>         +    void processInstruction(MachineBasicBlock::iterator& I,
>         +                            MachineFunction::iterator MFI);
>         +    RegUsageState usesRegister(MachineOperand& p,
>         +                               MachineBasicBlock::iterator
>         I);
>         +    MachineBasicBlock::iterator
>         searchBackwards(MachineOperand& p,
>         +
>                                                        MachineBasicBlock::iterator& I,
>         +
>                                                        MachineFunction::iterator MFI);
>         +    MachineInstr*
>         postRAConvertToLEA(MachineFunction::iterator &MFI,
>         +
>                                             MachineBasicBlock::iterator &MBBI,
>         +                                     LiveVariables *LV)
>         const;
>         +
>         
>         
>  
> 
> 
> Can you please document the declarations of these functions ?
> 
> 
> ( see
> http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments )
> 
> 
>  
> 
> 
>         
>         +
>         +/// postRAConvertToLEA - if an instruction can be converted
>         to an 
>         +/// equivalent LEA, insert the new instruction into the basic
>         block
>         +/// and return a pointer to it. Otherwise, return zero.
>         +MachineInstr *
>         +FixupLEAPass::postRAConvertToLEA(MachineFunction::iterator
>         &MFI,
>         +                                 MachineBasicBlock::iterator
>         &MBBI,
>         +                                 LiveVariables *LV) const {
>         
>         
>  
> 
> 
>  
> 
> 
> Preston,  LiveVariables are not available in this phase and it is only
> confusing.  Please remove them from the code. 
> 
> 
>  
> 
> 
> +  LV = getAnalysisIfAvailable<LiveVariables>();  <----- this will
> always return 0.  Did you ever get a LiveVariable instance ?
> 
> 
>  
> 
> 
>         +  // Process all basic blocks.
>         +  for (MachineFunction::iterator I = Func.begin(), E =
>         Func.end(); I != E; ++I)
>         +    processBasicBlock(Func, I);
>         +  DEBUG(dbgs() << "End X86FixupLEAs\n";);
>         +
>         +  return true;
>         +}
>         
>         
>  
> 
> 
>  
> 
> 
> This function always returns true.  You probably only want to return
> true of the code changed. 
> 
> 
>  
> 
> 
> Can you reduce the size of the test cases ?  Ideally I would like to
> see a single-basic block function. You only need 5-instructions or so.
> Why did you include such long tests ?
> 
> 
>  
> 
> 
>  
> 
> 
>  
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits