[llvm] r273899 - X86 Lowering - Fixed a crash in ICMP scalar instruction

[Quentin Colombet via llvm-commits]

Hi Elena,

Couple of comments inlined.

> On Jun 27, 2016, at 11:07 AM, Elena Demikhovsky via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Author: delena
> Date: Mon Jun 27 13:07:16 2016
> New Revision: 273899
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=273899&view=rev
> Log:
> X86 Lowering - Fixed a crash in ICMP scalar instruction
> 
> Fixed a bug in EmitTest() function in combining shl + icmp.
> 
> https://llvm.org/bugs/show_bug.cgi?id=28119
> 
> 
> Added:
>    llvm/trunk/test/CodeGen/X86/2016-06-28-ICmpCrash.ll
> Modified:
>    llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> 
> Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=273899&r1=273898&r2=273899&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Mon Jun 27 13:07:16 2016
> @@ -14597,10 +14597,8 @@ SDValue X86TargetLowering::EmitTest(SDVa
>                        : APInt::getLowBitsSet(BitWidth, BitWidth - ShAmt);
>       if (!Mask.isSignedIntN(32)) // Avoid large immediates.
>         break;
> -      SDValue New = DAG.getNode(ISD::AND, dl, VT, Op->getOperand(0),
> -                                DAG.getConstant(Mask, dl, VT));
> -      DAG.ReplaceAllUsesWith(Op, New);
> -      Op = New;
> +      Op = DAG.getNode(ISD::AND, dl, VT, Op->getOperand(0),
> +                       DAG.getConstant(Mask, dl, VT));
>     }
>     break;
> 
> 
> Added: llvm/trunk/test/CodeGen/X86/2016-06-28-ICmpCrash.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/2016-06-28-ICmpCrash.ll?rev=273899&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/2016-06-28-ICmpCrash.ll (added)
> +++ llvm/trunk/test/CodeGen/X86/2016-06-28-ICmpCrash.ll Mon Jun 27 13:07:16 2016

1. Do not put the date in the file name. This is a pain to look for specific file later on.

2. Couldn’t you merge this test with an existing file? I believe we should have avx2 check for the lowering of icmp, right?

> @@ -0,0 +1,31 @@
> +;RUN: llc < %s -mcpu=core-avx2

3. Please use FileCheck to make sure we generate something sane.
Not crashing does not mean we generate correct code!

4. Please add a comment on what this test is actually testing.
This is useful to know how to update the test when it starts to fail.

> +
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> +%struct.struct_1 = type { i24 }
> +
> + at a = global i8 0, align 1
> + at b = global i8 0, align 1
> + at d = global i8 0, align 1
> + at e = global i8 0, align 1
> + at c = global %struct.struct_1 zeroinitializer, align 4
> +
> +; Function Attrs: norecurse nounwind uwtable
> +define void @_Z3fn1v() #0 {
> +  %bf.load = load i32, i32* bitcast (%struct.struct_1* @c to i32*), align 4
> +  %bf.shl = shl i32 %bf.load, 8
> +  %bf.ashr = ashr exact i32 %bf.shl, 8
> +  %tobool4 = icmp ne i32 %bf.ashr, 0
> +  %conv = zext i1 %tobool4 to i32
> +  %x1 = load i8, i8* @e, align 1
> +  %conv6 = zext i8 %x1 to i32
> +  %add = add nuw nsw i32 %conv, %conv6
> +  %tobool7 = icmp ne i32 %add, 0
> +  %frombool = zext i1 %tobool7 to i8
> +  store i8 %frombool, i8* @b, align 1
> +  %tobool14 = icmp ne i32 %bf.shl, 0
> +  %frombool15 = zext i1 %tobool14 to i8
> +  store i8 %frombool15, i8* @d, align 1
> +  ret void
> +}
> \ No newline at end of file

Add a newline.

Cheers,
-Quentin

> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits