[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]

Ok, thanks Dan!

On Oct 29, 2009, at 1:44 PM, Dan Gohman wrote:

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