[llvm-commits] [llvm] r146604 - in /llvm/trunk: lib/CodeGen/MachineSink.cpp test/CodeGen/ARM/2011-12-14-machine-sink.ll
Devang Patel
dpatel at apple.com
Tue Dec 20 15:52:41 PST 2011
Akira,
On Dec 20, 2011, at 11:39 AM, Akira Hatanaka wrote:
> After further investigation, I found a function that is possibly being
> mis-compiled. This is what I did:
>
> 1. Reverted this patch (machine sinking is less conservative now).
> 2. Turned off machine sinking just for the function.
>
> The generated executable segfaults when it is run.
>
> Additionally, I found that it produces the expected result and
> terminates normally if I add the option "-regalloc=basic" to llc to
> turn off greedy register allocation.
>
> Is there anything you can tell from this? Which part of the output
> should I pay attention to when I am reading llc's outputs when option
> -debug or -stats is provided? The function in question is fairly large
> and complicated, so it is difficult to tell which part is being
> mis-compiled just by looking at the machinefunction dumps or the CFGs
> viewCFG and viewCFGOnly show.
One alternative for you is to reproduce segfault in debugger and investigate what is being compiled.
Other alternative is to narrow down the particular MI sink that is difference maker. You can do this by disabling this patch and adding a artificial threshold in Machine Sink pass to control how many instructions are sinked. One you know the critical MI sink, you can investigate how it impacts other passes, including register allocator, down the road.
I am afraid, there is not any easy answer.
-
Devang
>
> On Fri, Dec 16, 2011 at 3:50 PM, Devang Patel <dpatel at apple.com> wrote:
>>
>> On Dec 16, 2011, at 2:34 PM, Akira Hatanaka wrote:
>>
>>> The test still fails after I disable Machine Sink pass, but it passes
>>> if I revert the patch (just this one, the ones checked in after this
>>> remain applied). So it looks like MachineSinking is not causing the
>>> failure, but there is a bug in some other part of the backend that
>>> gets exposed if the pass doesn't sink instructions at all or is less
>>> aggresive.
>>>
>>> Do you have any idea which pass might be doing something wrong?
>>
>>
>> In the test case, which prompted this patch, machine sink pass was increasing distance between reg def and use resulting in unnecessary extra spill.
>>
>> However, in your particular case I have no idea what is going wrong. Finding out what exactly is miscompiled may be your best alternative. Do you see any significant (or noticeable) changes in stats for passes that follow machine sink ?
>>>
>>> Here are the stats:
>>>
>>> (before)
>>> 15 machine-sink - Number of copies coalesced
>>> 674 machine-sink - Number of critical edges split
>>> 3397 machine-sink - Number of machine instructions sunk
>>>
>>> (after)
>>> 15 machine-sink - Number of copies coalesced
>>> 674 machine-sink - Number of critical edges split
>>> 2881 machine-sink - Number of machine instructions sunk
>>>
>>> On Fri, Dec 16, 2011 at 9:21 AM, Devang Patel <dpatel at apple.com> wrote:
>>>>
>>>> On Dec 15, 2011, at 7:55 PM, Akira Hatanaka wrote:
>>>>
>>>>> This commit is causing one the tests in llvm test-suite
>>>>> (MultiSource/Benchmarks/tramp3d-v4) to fail.
>>>>> It segfaults during execution. The target architecture is mips32r2.
>>>>>
>>>>> Would it be possible to revert this until we can figure out what is
>>>>> causing the failure?
>>>>
>>>> This patch makes machine sink more conservative. Please,
>>>>
>>>> 1) Disable Machine Sink pass completely locally and see if the segfaults reproduces or not.
>>>> 2) Enable stats and see if notice any change before and after the patch.
>>>> 3) Please file bugzilla with a reproducible test case before reverting the patch.
>>>>
>>>> Thanks,
>>>> -
>>>> Devang
>>>>
>>>>
>>>>> On Wed, Dec 14, 2011 at 3:20 PM, Devang Patel <dpatel at apple.com> wrote:
>>>>>> Author: dpatel
>>>>>> Date: Wed Dec 14 17:20:38 2011
>>>>>> New Revision: 146604
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=146604&view=rev
>>>>>> Log:
>>>>>> Do not sink instruction, if it is not profitable.
>>>>>>
>>>>>> On ARM, peephole optimization for ABS creates a trivial cfg triangle which tempts machine sink to sink instructions in code which is really straight line code. Sometimes this sinking may alter register allocator input such that use and def of a reg is divided by a branch in between, which may result in extra spills. Now mahine sink avoids sinking if final sink destination is post dominator.
>>>>>>
>>>>>> Radar 10266272.
>>>>>>
>>>>>> Added:
>>>>>> llvm/trunk/test/CodeGen/ARM/2011-12-14-machine-sink.ll
>>>>>> Modified:
>>>>>> llvm/trunk/lib/CodeGen/MachineSink.cpp
>>>>>>
>>>>>> Modified: llvm/trunk/lib/CodeGen/MachineSink.cpp
>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineSink.cpp?rev=146604&r1=146603&r2=146604&view=diff
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/lib/CodeGen/MachineSink.cpp (original)
>>>>>> +++ llvm/trunk/lib/CodeGen/MachineSink.cpp Wed Dec 14 17:20:38 2011
>>>>>> @@ -90,7 +90,11 @@
>>>>>> bool AllUsesDominatedByBlock(unsigned Reg, MachineBasicBlock *MBB,
>>>>>> MachineBasicBlock *DefMBB,
>>>>>> bool &BreakPHIEdge, bool &LocalUse) const;
>>>>>> - MachineBasicBlock *FindSuccToSinkTo(MachineInstr *MI, bool &BreakPHIEdge);
>>>>>> + MachineBasicBlock *FindSuccToSinkTo(MachineInstr *MI, MachineBasicBlock *MBB,
>>>>>> + bool &BreakPHIEdge);
>>>>>> + bool isProfitableToSinkTo(unsigned Reg, MachineInstr *MI,
>>>>>> + MachineBasicBlock *MBB,
>>>>>> + MachineBasicBlock *SuccToSinkTo);
>>>>>>
>>>>>> bool PerformTrivialForwardCoalescing(MachineInstr *MI,
>>>>>> MachineBasicBlock *MBB);
>>>>>> @@ -399,18 +403,76 @@
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +/// isPostDominatedBy - Return true if A is post dominated by B.
>>>>>> +static bool isPostDominatedBy(MachineBasicBlock *A, MachineBasicBlock *B) {
>>>>>> +
>>>>>> + // FIXME - Use real post dominator.
>>>>>> + if (A->succ_size() != 2)
>>>>>> + return false;
>>>>>> + MachineBasicBlock::succ_iterator I = A->succ_begin();
>>>>>> + if (B == *I)
>>>>>> + ++I;
>>>>>> + MachineBasicBlock *OtherSuccBlock = *I;
>>>>>> + if (OtherSuccBlock->succ_size() != 1 ||
>>>>>> + *(OtherSuccBlock->succ_begin()) != B)
>>>>>> + return false;
>>>>>> +
>>>>>> + return true;
>>>>>> +}
>>>>>> +
>>>>>> +/// isProfitableToSinkTo - Return true if it is profitable to sink MI.
>>>>>> +bool MachineSinking::isProfitableToSinkTo(unsigned Reg, MachineInstr *MI,
>>>>>> + MachineBasicBlock *MBB,
>>>>>> + MachineBasicBlock *SuccToSinkTo) {
>>>>>> + assert (MI && "Invalid MachineInstr!");
>>>>>> + assert (SuccToSinkTo && "Invalid SinkTo Candidate BB");
>>>>>> +
>>>>>> + if (MBB == SuccToSinkTo)
>>>>>> + return false;
>>>>>> +
>>>>>> + // It is profitable if SuccToSinkTo does not post dominate current block.
>>>>>> + if (!isPostDominatedBy(MBB, SuccToSinkTo))
>>>>>> + return true;
>>>>>> +
>>>>>> + // Check if only use in post dominated block is PHI instruction.
>>>>>> + bool NonPHIUse = false;
>>>>>> + for (MachineRegisterInfo::use_nodbg_iterator
>>>>>> + I = MRI->use_nodbg_begin(Reg), E = MRI->use_nodbg_end();
>>>>>> + I != E; ++I) {
>>>>>> + MachineInstr *UseInst = &*I;
>>>>>> + MachineBasicBlock *UseBlock = UseInst->getParent();
>>>>>> + if (UseBlock == SuccToSinkTo && !UseInst->isPHI())
>>>>>> + NonPHIUse = true;
>>>>>> + }
>>>>>> + if (!NonPHIUse)
>>>>>> + return true;
>>>>>> +
>>>>>> + // If SuccToSinkTo post dominates then also it may be profitable if MI
>>>>>> + // can further profitably sinked into another block in next round.
>>>>>> + bool BreakPHIEdge = false;
>>>>>> + // FIXME - If finding successor is compile time expensive then catch results.
>>>>>> + if (MachineBasicBlock *MBB2 = FindSuccToSinkTo(MI, SuccToSinkTo, BreakPHIEdge))
>>>>>> + return isProfitableToSinkTo(Reg, MI, SuccToSinkTo, MBB2);
>>>>>> +
>>>>>> + // If SuccToSinkTo is final destination and it is a post dominator of current
>>>>>> + // block then it is not profitable to sink MI into SuccToSinkTo block.
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> /// FindSuccToSinkTo - Find a successor to sink this instruction to.
>>>>>> MachineBasicBlock *MachineSinking::FindSuccToSinkTo(MachineInstr *MI,
>>>>>> - bool &BreakPHIEdge) {
>>>>>> + MachineBasicBlock *MBB,
>>>>>> + bool &BreakPHIEdge) {
>>>>>> +
>>>>>> + assert (MI && "Invalid MachineInstr!");
>>>>>> + assert (MBB && "Invalid MachineBasicBlock!");
>>>>>>
>>>>>> // Loop over all the operands of the specified instruction. If there is
>>>>>> // anything we can't handle, bail out.
>>>>>> - MachineBasicBlock *ParentBlock = MI->getParent();
>>>>>>
>>>>>> // SuccToSinkTo - This is the successor to sink this instruction to, once we
>>>>>> // decide.
>>>>>> MachineBasicBlock *SuccToSinkTo = 0;
>>>>>> -
>>>>>> for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
>>>>>> const MachineOperand &MO = MI->getOperand(i);
>>>>>> if (!MO.isReg()) continue; // Ignore non-register operands.
>>>>>> @@ -469,7 +531,7 @@
>>>>>> // If a previous operand picked a block to sink to, then this operand
>>>>>> // must be sinkable to the same block.
>>>>>> bool LocalUse = false;
>>>>>> - if (!AllUsesDominatedByBlock(Reg, SuccToSinkTo, ParentBlock,
>>>>>> + if (!AllUsesDominatedByBlock(Reg, SuccToSinkTo, MBB,
>>>>>> BreakPHIEdge, LocalUse))
>>>>>> return NULL;
>>>>>>
>>>>>> @@ -478,11 +540,11 @@
>>>>>>
>>>>>> // Otherwise, we should look at all the successors and decide which one
>>>>>> // we should sink to.
>>>>>> - for (MachineBasicBlock::succ_iterator SI = ParentBlock->succ_begin(),
>>>>>> - E = ParentBlock->succ_end(); SI != E; ++SI) {
>>>>>> - MachineBasicBlock *SuccBlock = *SI;
>>>>>> + for (MachineBasicBlock::succ_iterator SI = MBB->succ_begin(),
>>>>>> + E = MBB->succ_end(); SI != E; ++SI) {
>>>>>> + MachineBasicBlock *SuccBlock = *SI;
>>>>>> bool LocalUse = false;
>>>>>> - if (AllUsesDominatedByBlock(Reg, SuccBlock, ParentBlock,
>>>>>> + if (AllUsesDominatedByBlock(Reg, SuccBlock, MBB,
>>>>>> BreakPHIEdge, LocalUse)) {
>>>>>> SuccToSinkTo = SuccBlock;
>>>>>> break;
>>>>>> @@ -495,12 +557,14 @@
>>>>>> // If we couldn't find a block to sink to, ignore this instruction.
>>>>>> if (SuccToSinkTo == 0)
>>>>>> return NULL;
>>>>>> + else if (!isProfitableToSinkTo(Reg, MI, MBB, SuccToSinkTo))
>>>>>> + return NULL;
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> // It is not possible to sink an instruction into its own block. This can
>>>>>> // happen with loops.
>>>>>> - if (ParentBlock == SuccToSinkTo)
>>>>>> + if (MBB == SuccToSinkTo)
>>>>>> return NULL;
>>>>>>
>>>>>> // It's not safe to sink instructions to EH landing pad. Control flow into
>>>>>> @@ -532,7 +596,8 @@
>>>>>> // and z and only shrink the live range of x.
>>>>>>
>>>>>> bool BreakPHIEdge = false;
>>>>>> - MachineBasicBlock *SuccToSinkTo = FindSuccToSinkTo(MI, BreakPHIEdge);
>>>>>> + MachineBasicBlock *ParentBlock = MI->getParent();
>>>>>> + MachineBasicBlock *SuccToSinkTo = FindSuccToSinkTo(MI, ParentBlock, BreakPHIEdge);
>>>>>>
>>>>>> // If there are no outputs, it must have side-effects.
>>>>>> if (SuccToSinkTo == 0)
>>>>>> @@ -553,8 +618,6 @@
>>>>>>
>>>>>> DEBUG(dbgs() << "Sink instr " << *MI << "\tinto block " << *SuccToSinkTo);
>>>>>>
>>>>>> - MachineBasicBlock *ParentBlock = MI->getParent();
>>>>>> -
>>>>>> // If the block has multiple predecessors, this would introduce computation on
>>>>>> // a path that it doesn't already exist. We could split the critical edge,
>>>>>> // but for now we just punt.
>>>>>>
>>>>>> Added: llvm/trunk/test/CodeGen/ARM/2011-12-14-machine-sink.ll
>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/2011-12-14-machine-sink.ll?rev=146604&view=auto
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/test/CodeGen/ARM/2011-12-14-machine-sink.ll (added)
>>>>>> +++ llvm/trunk/test/CodeGen/ARM/2011-12-14-machine-sink.ll Wed Dec 14 17:20:38 2011
>>>>>> @@ -0,0 +1,48 @@
>>>>>> +; RUN: llc < %s -o /dev/null -stats |& FileCheck %s -check-prefix=STATS
>>>>>> +; Radar 10266272
>>>>>> +target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:32:64-v128:32:128-a0:0:32-n32-S32"
>>>>>> +target triple = "thumbv7-apple-ios4.0.0"
>>>>>> +; STATS-NOT: machine-sink
>>>>>> +
>>>>>> +define i32 @foo(i32 %h) nounwind readonly ssp {
>>>>>> +entry:
>>>>>> + br label %for.cond
>>>>>> +
>>>>>> +for.cond: ; preds = %for.body, %entry
>>>>>> + %cmp = icmp slt i32 0, %h
>>>>>> + br i1 %cmp, label %for.body, label %if.end299
>>>>>> +
>>>>>> +for.body: ; preds = %for.cond
>>>>>> + %v.5 = select i1 undef, i32 undef, i32 0
>>>>>> + %0 = load i8* undef, align 1, !tbaa !0
>>>>>> + %conv88 = zext i8 %0 to i32
>>>>>> + %sub89 = sub nsw i32 0, %conv88
>>>>>> + %v.8 = select i1 undef, i32 undef, i32 %sub89
>>>>>> + %1 = load i8* null, align 1, !tbaa !0
>>>>>> + %conv108 = zext i8 %1 to i32
>>>>>> + %2 = load i8* undef, align 1, !tbaa !0
>>>>>> + %conv110 = zext i8 %2 to i32
>>>>>> + %sub111 = sub nsw i32 %conv108, %conv110
>>>>>> + %cmp112 = icmp slt i32 %sub111, 0
>>>>>> + %sub115 = sub nsw i32 0, %sub111
>>>>>> + %v.10 = select i1 %cmp112, i32 %sub115, i32 %sub111
>>>>>> + %add62 = add i32 0, %v.5
>>>>>> + %add73 = add i32 %add62, 0
>>>>>> + %add84 = add i32 %add73, 0
>>>>>> + %add95 = add i32 %add84, %v.8
>>>>>> + %add106 = add i32 %add95, 0
>>>>>> + %add117 = add i32 %add106, %v.10
>>>>>> + %add128 = add i32 %add117, 0
>>>>>> + %add139 = add i32 %add128, 0
>>>>>> + %add150 = add i32 %add139, 0
>>>>>> + %add161 = add i32 %add150, 0
>>>>>> + %add172 = add i32 %add161, 0
>>>>>> + br i1 undef, label %for.cond, label %if.end299
>>>>>> +
>>>>>> +if.end299: ; preds = %for.body, %for.cond
>>>>>> + %s.10 = phi i32 [ %add172, %for.body ], [ 0, %for.cond ]
>>>>>> + ret i32 %s.10
>>>>>> +}
>>>>>> +
>>>>>> +!0 = metadata !{metadata !"omnipotent char", metadata !1}
>>>>>> +!1 = metadata !{metadata !"Simple C/C++ TBAA", null}
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>
More information about the llvm-commits
mailing list