[llvm-commits] [llvm] r171879 - in /llvm/trunk: lib/Target/X86/CMakeLists.txt lib/Target/X86/X86.h lib/Target/X86/X86.td lib/Target/X86/X86PadShortFunction.cpp lib/Target/X86/X86Subtarget.cpp lib/Target/X86/X86Subtarget.h lib/Target/X86/X86TargetMa...

[Zhang, Andy]

On January 08, 2013 3:28 PM, Evan Cheng wrote:

Hi Evan,

> 1. It's checking for -Os, but not -Oz.

Is it necessary to explicitly check for -Oz? Doesn't -Oz also set the optforsize attribute?

> 2. This code looks like compile time intensive. It's walking the CFG but it
> doesn't track what BBs have been visited already. It definitely looks like
> it can visit a single BB multiple times and does wasted computation.

The code will only walk the CFG until the cycle count threshold is reached (currently 4 cycles). I didn't think it was necessary to track which BBs were visited given how few instructions are examined.

> 3. Other issues:
> 
> +    if (Cycles < Threshold) {
> +      if (!cyclesUntilReturn(MBB, BBCycles, &ReturnLoc))
> +        continue;
> 
> Why is it calling cyclesUntilReturn again? Isn't it called by findReturns()
> already?
 
> Why is it necessary to save Location? The return instruction is always at
> the end of the BB.

You're right, the ret is always at the end of the BB so we don't need to save Location. The second call to cyclesUntilReturn() was just to get a MBBI to insert the NOOPs. I've attached a patch to remove this part. Does it look ok now?

> 4. The whole approach just seems weird to me. You should only pad extremely
> short functions  (4 instructions?) but the code has to scan the entire CFG.
> I think you are essentially just looking for functions where there is an
> early branch from the entry BB to the exit BB.

It's not just functions that have an early exit, but any function that is short. The function might have no control flow but still be less than 4 cycles long. It won't scan the entire CFG unless the function is short to begin with.


Regards,
Andy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pad.diff
Type: application/octet-stream
Size: 2715 bytes
Desc: pad.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130108/706de256/attachment.obj>