[PATCH] Fix crash in MachineLICM.cpp

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 8 16:41:58 PDT 2016


Could you make a patch out of the test case + the code change?

A couple more comment on the test case:
1. Use 'opt -instnamer' to get rid of the %[0-9]+ variables (you could do that manually since you only have one %0).
2. Add a comment explaining what this test is checking.
3. Add more check lines to make sure we generate something sane. (Like add a check for load and add, or whatever was touched by the LICM pass.)
4. You may want to use a triple instead of march for stability accross bot.

Cheers,
Q.
> On Jun 8, 2016, at 4:34 PM, zan jyu Wong <zyfwong at gmail.com> wrote:
> 
> Hi,
> 
> The following is the test:
> 
> ; RUN: llc -march=thumb -sink-insts-to-avoid-spills -o - %s \
> ; RUN:     | FileCheck %s
> ; CHECK-LABEL: sum
> 
> ; Function Attrs: nounwind readonly
> define arm_aapcscc i32 @sum(i32* noalias nocapture readonly %arr, i32 %c) {
> entry:
>   %cmp7 = icmp sgt i32 %c, 0
>   br i1 %cmp7, label %for.body.preheader, label %for.cond.cleanup
> 
> for.body.preheader:                               ; preds = %entry
>   %sp = tail call i32 asm "mov $0, sp", "=r"()
>   br label %for.body
> 
> for.cond.cleanup.loopexit:                        ; preds = %for.body
>   br label %for.cond.cleanup
> 
> for.cond.cleanup:                                 ; preds = %for.cond.cleanup.loopexit, %entry
>   %sum.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.cond.cleanup.loopexit ]
>   ret i32 %sum.0.lcssa
> 
> for.body:                                         ; preds = %for.body, %for.body.preheader
>   %i.09 = phi i32 [ %inc, %for.body ], [ 0, %for.body.preheader ]
>   %sum.08 = phi i32 [ %add, %for.body ], [ 0, %for.body.preheader ]
>   %arrayidx = getelementptr inbounds i32, i32* %arr, i32 %i.09
>   %0 = load i32, i32* %arrayidx, align 4
>   %add = add nsw i32 %0, %sp
>   %inc = add nuw nsw i32 %i.09, 1
>   %exitcond = icmp eq i32 %inc, %c
>   br i1 %exitcond, label %for.cond.cleanup.loopexit, label %for.body
> }
> 
> On Thu, Jun 9, 2016 at 6:06 AM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
> Hi Zan Jyu,
> 
> Great!
> 
> Add a few CHECK lines and get rid of the unused metadata and you should be good to go!
> 
> Cheers,
> -Quentin
> 
>> On Jun 7, 2016, at 9:06 PM, zan jyu Wong <zyfwong at gmail.com <mailto:zyfwong at gmail.com>> wrote:
>> 
>> I've tried the following example on Thumb backend with the flags, it's ok?
>> 
>> RUN: llc licm.ll -sink-insts-to-avoid-spills
>> 
>> ========== example ===========
>> ; ModuleID = 'licm.c'
>> source_filename = "licm.c"
>> target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
>> target triple = "armv4t--"
>> 
>> ; Function Attrs: nounwind readonly
>> define arm_aapcscc i32 @sum(i32* noalias nocapture readonly %arr, i32 %c) #0 {
>> entry:
>>   %cmp7 = icmp sgt i32 %c, 0
>>   br i1 %cmp7, label %for.body.preheader, label %for.cond.cleanup
>> 
>> for.body.preheader:                               ; preds = %entry
>>   %sp = tail call i32 asm "mov $0, sp", "=r"() #1, !srcloc !3
>>   br label %for.body
>> 
>> for.cond.cleanup.loopexit:                        ; preds = %for.body
>>   br label %for.cond.cleanup
>> 
>> for.cond.cleanup:                                 ; preds = %for.cond.cleanup.loopexit, %entry
>>   %sum.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.cond.cleanup.loopexit ]
>>   ret i32 %sum.0.lcssa
>> 
>> for.body:                                         ; preds = %for.body.preheader, %for.body
>>   %i.09 = phi i32 [ %inc, %for.body ], [ 0, %for.body.preheader ]
>>   %sum.08 = phi i32 [ %add, %for.body ], [ 0, %for.body.preheader ]
>>   %arrayidx = getelementptr inbounds i32, i32* %arr, i32 %i.09
>>   %0 = load i32, i32* %arrayidx, align 4, !tbaa !4
>>   %mul = mul nsw i32 %0, %sp
>>   %add = add nsw i32 %mul, %sum.08
>>   %inc = add nuw nsw i32 %i.09, 1
>>   %exitcond = icmp eq i32 %inc, %c
>>   br i1 %exitcond, label %for.cond.cleanup.loopexit, label %for.body
>> }
>> 
>> attributes #0 = { nounwind readonly "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="arm7tdmi" "target-features"="+soft-float,+strict-align,-crypto,-neon" "unsafe-fp-math"="false" "use-soft-float"="true" }
>> attributes #1 = { nounwind readnone }
>> 
>> !llvm.module.flags = !{!0, !1}
>> !llvm.ident = !{!2}
>> 
>> !0 = !{i32 1, !"wchar_size", i32 4}
>> !1 = !{i32 1, !"min_enum_size", i32 4}
>> !2 = !{!"clang version 3.9.0 (http://llvm.org/git/clang.git <http://llvm.org/git/clang.git> e8204bf8264d6d52aab4e8ac84328654d3ef28bd) (http://llvm.org/git/llvm.git <http://llvm.org/git/llvm.git> d75f887d2fcccc1f0dbf7809c2638e2ff3448462)"}
>> !3 = !{i32 76}
>> !4 = !{!5, !5, i64 0}
>> !5 = !{!"long", !6, i64 0}
>> !6 = !{!"omnipotent char", !7, i64 0}
>> !7 = !{!"Simple C/C++ TBAA"}
>> 
>> On Wed, Jun 8, 2016 at 8:50 AM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>> Could you try a simple example with that flag on x86_64 to produce a test case?
>> 
>> Thanks,
>> Q.
>> 
>>> On Jun 7, 2016, at 5:38 PM, zan jyu Wong <zyfwong at gmail.com <mailto:zyfwong at gmail.com>> wrote:
>>> 
>>> No. Since I'm working on a custom backend, unless I upload the backend.
>>> But there is an assertion in isDef(), so I think it can be reproduced on other backend when compile with -sink-insts-to-avoid-spills=true.
>>> 
>>> On Wed, Jun 8, 2016 at 12:03 AM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>>> Hi Zan Jyu,
>>> 
>>> Yep.
>>> 
>>> Do you have a test case to put with that?
>>> 
>>> Thanks,
>>> -Quentin
>>>> On Jun 7, 2016, at 12:25 AM, zan jyu Wong via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>> 
>>>> We should test isReg() before isDef()
>>>> 
>>>> diff --git a/lib/CodeGen/MachineLICM.cpp b/lib/CodeGen/MachineLICM.cpp
>>>> index 00dec82..4ea0c56 100644
>>>> --- a/lib/CodeGen/MachineLICM.cpp
>>>> +++ b/lib/CodeGen/MachineLICM.cpp
>>>> @@ -716,7 +716,7 @@ void MachineLICM::SinkIntoLoop() {
>>>>  
>>>>    for (MachineInstr *I : Candidates) {
>>>>      const MachineOperand &MO = I->getOperand(0);
>>>> -    if (!MO.isDef() || !MO.isReg() || !MO.getReg())
>>>> +    if (!MO.isReg() || !MO.isDef() || !MO.getReg())
>>>>        continue;
>>>>      if (!MRI->hasOneDef(MO.getReg()))
>>>>        continue;
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>> 
>>> 
>> 
>> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160608/fc50b2dc/attachment.html>


More information about the llvm-commits mailing list