[llvm] r180573 - This patch adds the X86FixupLEAs pass, which will reduce instruction
Michael Liao
michael.liao at intel.com
Thu Apr 25 14:52:42 PDT 2013
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
More information about the llvm-commits
mailing list