[llvm-commits] [llvm] r141569 - in /llvm/trunk: lib/CodeGen/MachineLICM.cpp test/CodeGen/ARM/lsr-unfolded-offset.ll test/CodeGen/X86/licm-dominance.ll test/CodeGen/X86/licm-nested.ll

[Dan Gohman]

On Oct 10, 2011, at 12:09 PM, Devang Patel wrote:

> Author: dpatel
> Date: Mon Oct 10 14:09:20 2011
> New Revision: 141569
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=141569&view=rev
> Log:
> Add dominance check for the instruction being hoisted.
> 
> For example, MachineLICM should not hoist a load that is not guaranteed to be executed.
> Radar 10254254.
> 
> Added:
>    llvm/trunk/test/CodeGen/X86/licm-dominance.ll
> Modified:
>    llvm/trunk/lib/CodeGen/MachineLICM.cpp
>    llvm/trunk/test/CodeGen/ARM/lsr-unfolded-offset.ll
>    llvm/trunk/test/CodeGen/X86/licm-nested.ll
> 
> Modified: llvm/trunk/lib/CodeGen/MachineLICM.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineLICM.cpp?rev=141569&r1=141568&r2=141569&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineLICM.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineLICM.cpp Mon Oct 10 14:09:20 2011
> @@ -168,6 +168,11 @@
>     /// 
>     bool IsLoopInvariantInst(MachineInstr &I);
> 
> +    /// IsGuaranteedToExecute - check to make sure that the MI dominates
> +    /// all of the exit blocks.  If it doesn't, then there is a path out of the
> +    /// loop which does not execute this instruction, so we can't hoist it.
> +    bool IsGuaranteedToExecute(MachineInstr *MI);

The code is actually using the exiting blocks, not the exit blocks,
so please update the comments and names here and below.

> +
>     /// HasAnyPHIUse - Return true if the specified register is used by any
>     /// phi node.
>     bool HasAnyPHIUse(unsigned Reg) const;
> @@ -1129,6 +1134,29 @@
>   return false;
> }
> 
> +/// IsGuaranteedToExecute - check to make sure that the instruction dominates
> +/// all of the exit blocks.  If it doesn't, then there is a path out of the loop
> +/// which does not execute this instruction, so we can't hoist it.
> +bool MachineLICM::IsGuaranteedToExecute(MachineInstr *MI) {
> +  // If the instruction is in the header block for the loop (which is very
> +  // common), it is always guaranteed to dominate the exit blocks.  Since this
> +  // is a common case, and can save some work, check it now.
> +  if (MI->getParent() == CurLoop->getHeader())
> +    return true;
> +
> +  // Get the exit blocks for the current loop.
> +  SmallVector<MachineBasicBlock*, 8> ExitBlocks;
> +  CurLoop->getExitingBlocks(ExitBlocks);
> +
> +  // Verify that the block dominates each of the exit blocks of the loop.
> +  for (unsigned i = 0, e = ExitBlocks.size(); i != e; ++i)
> +    if (ExitBlocks[i] != CurLoop->getHeader() &&
> +	!DT->dominates(MI->getParent(), ExitBlocks[i]))

As we discussed, the ExitBlocks[i] != CurLoop->getHeader() check makes
this code miss cases.


> +      return false;
> +
> +  return true;
> +}
> +
> /// Hoist - When an instruction is found to use only loop invariant operands
> /// that are safe to hoist, this instruction is called to do the dirty work.
> ///
> @@ -1139,6 +1167,8 @@
>     MI = ExtractHoistableLoad(MI);
>     if (!MI) return false;
>   }
> +  if (!IsGuaranteedToExecute(MI))
> +    return false;

In Regular LICM, isGuaranteedToExecute is mediated by isSafeToSpeculativelyExecute.
Does it make sense to do that here too?

Also, this appears to apply to pre-RA MachineLICM. Does Post-RA MachineLICM
need this fix too?

Dan