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

[Dan Gohman]

On Oct 28, 2009, at 9:36 PM, Chris Lattner wrote:

> 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.

Yes, that is the idea.

>> +    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. :)

This one was a mistake; it's fixed now.

> 
>> +    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.

Fixed.

>> +    // 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?

Because that's all it would normally need? Anyway,
this code was removed in a subsequent simplification.

>> +++ 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).

I'd like to look into the hidden option to enable the feature which
fixes this.

Dan