[PATCH] Add logical ops to Mips fast-isel
Daniel Sanders
daniel.sanders at imgtec.com
Thu Feb 26 07:04:18 PST 2015
The code change looks right to me (assuming the answer to my main question below is 'no') but I have a couple questions and nits regarding the test case.
The main question is should we optimize 'X & 0', 'X | 0', and 'X ^ 0'?
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:22-48
@@ +21,29 @@
+
+; Function Attrs: noinline nounwind
+define void @setUb(i8 zeroext %x, i8 zeroext %y, i8 zeroext %z) #0 {
+entry:
+ store i8 %x, i8* @ub1, align 1
+ store i8 %y, i8* @ub2, align 1
+ store i8 %z, i8* @ub3, align 1
+ ret void
+}
+
+; Function Attrs: noinline nounwind
+define void @setUc(i8 zeroext %x, i8 zeroext %y, i8 zeroext %z) #0 {
+entry:
+ store i8 %x, i8* @uc1, align 1
+ store i8 %y, i8* @uc2, align 1
+ store i8 %z, i8* @uc3, align 1
+ ret void
+}
+
+; Function Attrs: noinline nounwind
+define void @setUs(i16 zeroext %x, i16 zeroext %y, i16 zeroext %z) #0 {
+entry:
+ store i16 %x, i16* @us1, align 2
+ store i16 %y, i16* @us2, align 2
+ store i16 %z, i16* @us3, align 2
+ ret void
+}
+
+; Function Attrs: noinline nounwind
----------------
Nit: These three aren't testing anything. They should be deleted.
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:50-72
@@ +49,25 @@
+; Function Attrs: noinline nounwind
+define void @andUb() #0 {
+entry:
+ %0 = load i8* @ub1, align 1
+ %1 = load i8* @ub2, align 1
+ %conv0 = trunc i8 %0 to i1
+ %conv1 = trunc i8 %1 to i1
+ %and0 = and i1 %conv1, %conv0
+ %conv3 = zext i1 %and0 to i8
+ store i8 %conv3, i8* @ub, align 1, !tbaa !2
+; CHECK: .ent andUb
+; CHECK: lui $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK: addiu $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
+; CHECK: addu $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
+; CHECK-DAG: lw $[[UB_ADDR:[0-9]+]], %got(ub)($[[REG_GP]])
+; CHECK-DAG: lw $[[UB2_ADDR:[0-9]+]], %got(ub2)($[[REG_GP]])
+; CHECK-DAG: lw $[[UB1_ADDR:[0-9]+]], %got(ub1)($[[REG_GP]])
+; CHECK-DAG: lbu $[[UB1:[0-9]+]], 0($[[UB1_ADDR]])
+; CHECK-DAG: lbu $[[UB2:[0-9]+]], 0($[[UB2_ADDR]])
+; CHECK-DAG: and $[[RES1:[0-9]+]], $[[UB2]], $[[UB1]]
+; CHECK: andi $[[RES:[0-9]+]], $[[RES1]], 1
+; CHECK: sb $[[RES]], 0($[[UB_ADDR]])
+ ret void
+}
+
----------------
Just a comment, no change required: This test and the others like it would be a lot simpler if it used arguments and returns. The patches needed for this appear later in the series though so this is ok for now and we should clean it up after those patches land.
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:59
@@ +58,3 @@
+ store i8 %conv3, i8* @ub, align 1, !tbaa !2
+; CHECK: .ent andUb
+; CHECK: lui $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
----------------
Nit: Please use CHECK-LABEL to partition the file
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:89
@@ +88,3 @@
+; CHECK-DAG: lbu $[[UB1:[0-9]+]], 0($[[UB1_ADDR]])
+; CHECK-DAG: and $[[RES1:[0-9]+]], $[[UB1]], $zero
+; CHECK: andi $[[RES:[0-9]+]], $[[RES1]], 1
----------------
There's a really trivial optimization opportunity here (x & 0 == 0). It would simply require an 'if (Opc == Mips::AND && RHSReg == Mips::ZERO) return RHSReg' in emitLogicalOp().
I'm wondering if we should handle it or not given that the goal is compile time. I'm leaning towards yes since it's so cheap to catch. Do we still delete dead code when using Fast ISel? If so, I think it's probably best to handle it since it removes the need to encode instructions. What do you think?
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:112
@@ +111,3 @@
+; CHECK-DAG: lbu $[[UB1:[0-9]+]], 0($[[UB1_ADDR]])
+; CHECK-DAG: and $[[RES1:[0-9]+]], $[[UB1]], $[[CONST]]
+; CHECK: andi $[[RES:[0-9]+]], $[[RES1]], 1
----------------
Why not 'andi $[[RES1:[0-9]+]], $[[UB1]], 1'? Will andi support be added in a later patch?
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:153-159
@@ +152,9 @@
+; CHECK: .ent orUb0
+; CHECK: lui $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK: addiu $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
+; CHECK: addu $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
+; CHECK-DAG: lw $[[UB_ADDR:[0-9]+]], %got(ub)($[[REG_GP]])
+; CHECK-DAG: lw $[[UB1_ADDR:[0-9]+]], %got(ub1)($[[REG_GP]])
+; CHECK-DAG: lbu $[[UB1:[0-9]+]], 0($[[UB1_ADDR]])
+; CHECK: andi $[[RES:[0-9]+]], $[[UB1]], 1
+; CHECK: sb $[[RES]], 0($[[UB_ADDR]])
----------------
Just a question: Any idea what optimized the 'X | 0'? I don't see anything in the patch that would do it and I'm surprised that it noticed this and didn't notice 'X & 0' or 'X ^ 0'.
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:228
@@ +227,3 @@
+; CHECK-DAG: lbu $[[UB1:[0-9]+]], 0($[[UB1_ADDR]])
+; CHECK-DAG: xor $[[RES1:[0-9]+]], $[[UB1]], $zero
+; CHECK: andi $[[RES:[0-9]+]], $[[RES1]], 1
----------------
Similar to the 'X & 0' case above, there's a trivial optimization opportunity here since 'X ^ 0 == X'. It would only require an 'if (Opc == Mips::XOR && RHSReg == Mips::ZERO) return LHSReg' in emitLogicalOp().
================
Comment at: test/CodeGen/Mips/Fast-ISel/logopm.ll:619-738
@@ +618,122 @@
+
+; Function Attrs: noinline nounwind
+;define i32 @main() #0 {
+;entry:
+; call void @setUb(i8 zeroext 1, i8 zeroext 1, i8 zeroext 1)
+; call void @andUb()
+; %0 = load i8* @ub, align 1, !tbaa !2
+; %conv = zext i8 %0 to i32
+; %call = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv) #2
+; call void @andUb0()
+; %1 = load i8* @ub, align 1, !tbaa !2
+; %conv1 = zext i8 %1 to i32
+; %call2 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv1) #2
+; call void @andUb1()
+; %2 = load i8* @ub, align 1, !tbaa !2
+; %conv3 = zext i8 %2 to i32
+; %call4 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv3) #2
+; call void @orUb()
+; %3 = load i8* @ub, align 1, !tbaa !2
+; %conv5 = zext i8 %3 to i32
+; %call6 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv5) #2
+; call void @orUb0()
+; %4 = load i8* @ub, align 1, !tbaa !2
+; %conv7 = zext i8 %4 to i32
+; %call8 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv7) #2
+; call void @orUb1()
+; %5 = load i8* @ub, align 1, !tbaa !2
+; %conv9 = zext i8 %5 to i32
+; %call10 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv9) #2
+; call void @xorUb()
+; %6 = load i8* @ub, align 1, !tbaa !2
+; %conv11 = zext i8 %6 to i32
+; %call12 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv11) #2
+; call void @xorUb0()
+; %7 = load i8* @ub, align 1, !tbaa !2
+; %conv13 = zext i8 %7 to i32
+; %call14 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv13) #2
+; call void @xorUb1()
+; %8 = load i8* @ub, align 1, !tbaa !2
+; %conv15 = zext i8 %8 to i32
+; %call16 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv15) #2
+; call void @setUc(i8 zeroext 74, i8 zeroext -31, i8 zeroext 52)
+; call void @andUc()
+; %9 = load i8* @uc, align 1, !tbaa !2
+; %conv17 = zext i8 %9 to i32
+; %call18 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv17) #2
+; call void @andUc0()
+; %10 = load i8* @uc, align 1, !tbaa !2
+; %conv19 = zext i8 %10 to i32
+; %call20 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv19) #2
+; call void @andUc1()
+; %11 = load i8* @uc, align 1, !tbaa !2
+; %conv21 = zext i8 %11 to i32
+; %call22 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv21) #2
+; call void @orUc()
+; %12 = load i8* @uc, align 1, !tbaa !2
+; %conv23 = zext i8 %12 to i32
+; %call24 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv23) #2
+; call void @orUc0()
+; %13 = load i8* @uc, align 1, !tbaa !2
+; %conv25 = zext i8 %13 to i32
+; %call26 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv25) #2
+; call void @orUc1()
+; %14 = load i8* @uc, align 1, !tbaa !2
+; %conv27 = zext i8 %14 to i32
+; %call28 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv27) #2
+; call void @xorUc()
+; %15 = load i8* @uc, align 1, !tbaa !2
+; %conv29 = zext i8 %15 to i32
+; %call30 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv29) #2
+; call void @xorUc0()
+; %16 = load i8* @uc, align 1, !tbaa !2
+; %conv31 = zext i8 %16 to i32
+; %call32 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv31) #2
+; call void @xorUc1()
+; %17 = load i8* @uc, align 1, !tbaa !2
+; %conv33 = zext i8 %17 to i32
+; %call34 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv33) #2
+; call void @setUs(i16 zeroext 19131, i16 zeroext 4833, i16 zeroext -24524)
+; call void @andUs()
+; %18 = load i16* @us, align 2, !tbaa !5
+; %conv35 = zext i16 %18 to i32
+; %call36 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv35) #2
+; call void @andUs0()
+; %19 = load i16* @us, align 2, !tbaa !5
+; %conv37 = zext i16 %19 to i32
+; %call38 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv37) #2
+; call void @andUs1()
+; %20 = load i16* @us, align 2, !tbaa !5
+; %conv39 = zext i16 %20 to i32
+; %call40 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv39) #2
+; call void @orUs()
+; %21 = load i16* @us, align 2, !tbaa !5
+; %conv41 = zext i16 %21 to i32
+; %call42 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv41) #2
+; call void @orUs0()
+; %22 = load i16* @us, align 2, !tbaa !5
+; %conv43 = zext i16 %22 to i32
+; %call44 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv43) #2
+; call void @orUs1()
+; %23 = load i16* @us, align 2, !tbaa !5
+; %conv45 = zext i16 %23 to i32
+; %call46 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv45) #2
+; call void @xorUs()
+; %24 = load i16* @us, align 2, !tbaa !5
+; %conv47 = zext i16 %24 to i32
+; %call48 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv47) #2
+; call void @xorUs0()
+; %25 = load i16* @us, align 2, !tbaa !5
+; %conv49 = zext i16 %25 to i32
+; %call50 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv49) #2
+; call void @xorUs1()
+; %26 = load i16* @us, align 2, !tbaa !5
+; %conv51 = zext i16 %26 to i32
+; %call52 = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 signext %conv51) #2
+; ret i32 0
+;}
+
+; Function Attrs: nounwind
+declare i32 @printf(i8* nocapture readonly, ...) #1
+
+attributes #0 = { noinline nounwind "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
Nit: Commented out code is discouraged. It should be deleted.
http://reviews.llvm.org/D6599
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list