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

[Evan Cheng]

There is a hidden x86 option that will re-enable the remat if you care  
about un-xfail this test.

Evan

On Oct 28, 2009, at 9:36 PM, Chris Lattner <clattner at apple.com> 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.
>
>> +    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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits