[ConstantHoisting] add XFAIL test case

[nickdesaulniers]

Summary

As reported by Linus Torvalds:
https://lore.kernel.org/lkml/CAHk-=[e-mail]/

[nickdesaulniers]

Hi @ributzka, do you have thoughts on how best to fix this issue demonstrated by this test case?

I have a patch that avoids hoisting to the same BB; not sure yet if there's perhaps a better approach.

[ributzka]

llvm/test/Transforms/ConstantHoisting/X86/bad-cases.ll

130

Why shouldn't this expensive constant be hoisted? The 68719476704 should be hoisted out of the BB and only the add should stay in %d. Why is 68719476704 materialized twice?

[nickdesaulniers]

llvm/test/Transforms/ConstantHoisting/X86/bad-cases.ll

130

Why shouldn't this expensive constant be hoisted?

I'm not even sure it's expensive on x86_64. Pretty sure it can fit in an immediate field for stores + comparisons.

The 68719476704 should be hoisted out of the BB and only the add should stay in %d.

Sure, that would have been acceptable, too.

Why is 68719476704 materialized twice?

That's what I'm curious about; this seems like a bug in ConstantHoist, right?

llvm/test/Transforms/ConstantHoisting/X86/bad-cases.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -passes=consthoist -S < %s | FileCheck %s
 target triple = "x86_64--"
 
 ; We don't want to convert constant divides because the benefit from converting
 ; them to a mul in the backend is larget than constant materialization savings.
 define void @signed_const_division(i64 %in1, i64 %in2, ptr %addr) {
 ; CHECK-LABEL: @signed_const_division(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
 ; CHECK:       loop:
 ; CHECK-NEXT:    [[L1:%.*]] = phi i64 [ [[RES1:%.*]], [[LOOP]] ], [ [[IN1:%.*]], [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    [[L2:%.*]] = phi i64 [ [[RES2:%.*]], [[LOOP]] ], [ [[IN2:%.*]], [[ENTRY]] ]
 ; CHECK-NEXT:    [[RES1]] = sdiv i64 [[L1]], 4294967296
-; CHECK-NEXT:    store volatile i64 [[RES1]], ptr [[ADDR:%.*]]
+; CHECK-NEXT:    store volatile i64 [[RES1]], ptr [[ADDR:%.*]], align 4
 ; CHECK-NEXT:    [[RES2]] = srem i64 [[L2]], 4294967296
-; CHECK-NEXT:    store volatile i64 [[RES2]], ptr [[ADDR]]
+; CHECK-NEXT:    store volatile i64 [[RES2]], ptr [[ADDR]], align 4
 ; CHECK-NEXT:    [[AGAIN:%.*]] = icmp eq i64 [[RES1]], [[RES2]]
 ; CHECK-NEXT:    br i1 [[AGAIN]], label [[LOOP]], label [[END:%.*]]
 ; CHECK:       end:
 ; CHECK-NEXT:    ret void
 ;
 entry:
   br label %loop
 

 define void @unsigned_const_division(i64 %in1, i64 %in2, ptr %addr) {
 ; CHECK-LABEL: @unsigned_const_division(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
 ; CHECK:       loop:
 ; CHECK-NEXT:    [[L1:%.*]] = phi i64 [ [[RES1:%.*]], [[LOOP]] ], [ [[IN1:%.*]], [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    [[L2:%.*]] = phi i64 [ [[RES2:%.*]], [[LOOP]] ], [ [[IN2:%.*]], [[ENTRY]] ]
 ; CHECK-NEXT:    [[RES1]] = udiv i64 [[L1]], 4294967296
-; CHECK-NEXT:    store volatile i64 [[RES1]], ptr [[ADDR:%.*]]
+; CHECK-NEXT:    store volatile i64 [[RES1]], ptr [[ADDR:%.*]], align 4
 ; CHECK-NEXT:    [[RES2]] = urem i64 [[L2]], 4294967296
-; CHECK-NEXT:    store volatile i64 [[RES2]], ptr [[ADDR]]
+; CHECK-NEXT:    store volatile i64 [[RES2]], ptr [[ADDR]], align 4
 ; CHECK-NEXT:    [[AGAIN:%.*]] = icmp eq i64 [[RES1]], [[RES2]]
 ; CHECK-NEXT:    br i1 [[AGAIN]], label [[LOOP]], label [[END:%.*]]
 ; CHECK:       end:
 ; CHECK-NEXT:    ret void
 ;
 
 entry:
   br label %loop
bb3:                                              ; No predecessors!
   br label %bb1
 
 bb5:                                              ; No predecessors!
   %tmp6 = load i32, ptr %tmp, align 4
   ret i32 %tmp6
 }
 
 declare i32 @c(...)
+
+; The intent of this test is to verify that 68719476705 does not get
+; transformed into 68719476704 + 1 in basic block %d.
+define i1 @foo (ptr %bar) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:    br i1 false, label [[A:%.*]], label [[B:%.*]]
+; CHECK:       a:
+; CHECK-NEXT:    [[CONST1:%.*]] = bitcast i64 68719476704 to i64
+; CHECK-NEXT:    [[UNUSED:%.*]] = icmp eq i64 0, [[CONST1]]
+; CHECK-NEXT:    ret i1 false
+; CHECK:       b:
+; CHECK-NEXT:    br i1 false, label [[C:%.*]], label [[D:%.*]]
+; CHECK:       c:
+; CHECK-NEXT:    ret i1 false
+; CHECK:       d:
+; CHECK-NEXT:    [[CONST:%.*]] = bitcast i64 68719476704 to i64
+; FIXME: this add is unnecessary and results in bad code gen. We should instead
+; have just emitted the bitcast to 68719476705 rather than 68719476704 + 1.
+; CHECK-NEXT:    [[CONST_MAT:%.*]] = add i64 [[CONST]], 1
+; CHECK-NEXT:    store i64 [[CONST_MAT]], ptr [[BAR:%.*]], align 4
+; CHECK-NEXT:    ret i1 false
+;
+  br i1 false, label %a, label %b
+a:
+  %unused = icmp eq i64 0, 68719476704
+  ret i1 false
+b:
+  br i1 false, label %c, label %d
+c:
+  ret i1 false
+d:
+  store i64 68719476705, ptr %bar
+  ret i1 false
+}