[llvm-commits] [llvm] r85364 - in /llvm/trunk: lib/CodeGen/MachineLICM.cpp test/CodeGen/X86/pic-load-remat.ll test/CodeGen/X86/sink-hoist.ll

[Chris Lattner]

On Oct 27, 2009, at 8:22 PM, Dan Gohman wrote:

> Author: djg
> Date: Tue Oct 27 22:21:57 2009
> New Revision: 85364
>
> URL: http://llvm.org/viewvc/llvm-project?rev=85364&view=rev
> Log:
> Teach MachineLICM to unfold loads from constant memory from
> otherwise unhoistable instructions in order to allow the loads
> to be hoisted.

Nice.

> +    if (const PseudoSourceValue *PSV =
> +          dyn_cast<PseudoSourceValue>(MMO->getValue())) {
> +      if (!PSV->isConstant(MF.getFrameInfo())) return;
> +    } else {
> +      if (!AA->pointsToConstantMemory(MMO->getValue())) return;
> +    }

eww, yuck :).  MachineLICM shouldn't have this logic.. a mythical  
CodeGenAA pass should.  Not super time critical of course.

> +    SmallVector<MachineInstr *, 1> NewMIs;

There isn't much harm in making this size 2 (since that's what the  
code expects) or even 4 if you want to be crazy. :)

> +    bool Success =
> +      TII->unfoldMemoryOperand(MF, MI, Reg,
> +                               /*UnfoldLoad=*/true, /*UnfoldStore=*/ 
> false,
> +                               NewMIs);
> +    (void)Success;
> +    assert(Success &&
> +           "unfoldMemoryOperand failed when  
> getOpcodeAfterMemoryUnfold "
> +           "succeeded!");
> +    assert(NewMIs.size() == 2 &&
> +           "Unfolded a load into multiple instructions!");
> +    MachineBasicBlock *MBB = MI->getParent();
> +    MBB->insert(MI, NewMIs[0]);
> +    MBB->insert(MI, NewMIs[1]);
> +    MI->eraseFromParent();

Please factor the 'memory operand hoisting' logic *out* of 'Hoist'  
into a helper function.

> +    // If unfolding produced a load that wasn't loop-invariant or  
> profitable to
> +    // hoist, re-fold it to undo the damage.
> +    if (!IsLoopInvariantInst(*NewMIs[0]) || !IsProfitableToHoist 
> (*NewMIs[0])) {
> +      SmallVector<unsigned, 1> Ops;

Again, why size 1?

> +++ llvm/trunk/test/CodeGen/X86/pic-load-remat.ll Tue Oct 27  
> 22:21:57 2009
> @@ -1,4 +1,10 @@
> ; RUN: llc < %s -mtriple=i686-apple-darwin -mattr=+sse2 -relocation- 
> model=pic | grep psllw | grep pb
> +; XFAIL: *
> +
> +; This is XFAIL'd because MachineLICM is now hoisting all of the  
> loads, and the pic
> +; base appears killed in the entry block when remat is making its  
> decisions. Remat's
> +; simple heuristic decides against rematting because it doesn't  
> want to extend the
> +; live-range of the pic base; this isn't necessarily optimal.

Please don't leave this XFAIL unless you plan to 'fix' it.  Either  
change the expected output or remove the test (moving the issue to a  
readme).

-Chris