[SystemZ] wrong-code resulting from storing / loading of i1

[Quuxplusone]

Bugzilla Link	PR45511
Status	RESOLVED FIXED
Importance	P normal
Reported by	Jonas Paulsson (paulsson@linux.vnet.ibm.com)
Reported on	2020-04-13 02:28:49 -0700
Last modified on	2020-04-15 04:02:34 -0700
Version	10.0
Hardware	PC Linux
CC	llvm-bugs@lists.llvm.org, paulsson@linux.vnet.ibm.com, uweigand@de.ibm.com
Fixed by commit(s)
Attachments	main.f.ll (2519 bytes, text/plain)
Blocks
Blocked by
See also

Created attachment 23343
reduced test case (main with inlined functions)

This Csmith program:

int a = 0, b = 0, d = 0;
static int c = 2;
int *f = 0;
int *bPtr = &b;
long e = 0;
long* const ePtr = &e;

static void fn5() {
  f = &c;
  *bPtr = 0;
  *f = 0;
}
int *fn6() {
  *bPtr = 8;
  *ePtr = (1 == -1L / (1 ^ ((unsigned) c)));
  return &d;
}
int main() {
  fn5();
  f = fn6();
  printf("checksum = %X\n", e);
}

should print '0', but clang -O3 prints '1'.

fn5() and fn6() get inlined into main:

Before ISel:

define dso_local signext i32 @main() local_unnamed_addr #0 {
entry:
  %0 = load i32*, i32** @bPtr, align 8, !tbaa !2
  store i1 true, i1* @c, align 4                 ; i1:true
  store i32 8, i32* %0, align 4, !tbaa !6
  %.b = load i1, i1* @c, align 4                 ; i1
  %conv.i = select i1 %.b, i64 1, i64 3          ; -> i64 1
  %div.i = sdiv i64 -1, %conv.i                  ; -1 / 1 -> -1
  %cmp.i = icmp eq i64 %div.i, 1                 ; -> false
  %conv2.i = zext i1 %cmp.i to i64               ; -> i64 0
  store i64 %conv2.i, i64* @e, align 8, !tbaa !8
  store i32* @d, i32** @f, align 8, !tbaa !2
  %call1 = tail call signext i32 (i8*, ...) @printf ... %conv2.i ...
  ret i32 0
}

Initial selection DAG: %bb.0 'main:entry'
// 'true' is stored as 1, which is -1 if interpreted as signed.
...
t7: ch = store<(store 1 into @c, align 4)> t4:1, Constant:i1<-1>, ... : i1:1
t10: i1,ch = load<(dereferenceable load 1 from @c, align 4)> t9, ...  : i1:1
       t13: i64 = select t10, Constant:i64<1>, Constant:i64<3>  : -> 1
     t15: i64 = sdiv Constant:i64<-1>, t13                      : -> -1
   t17: i1 = setcc t15, Constant:i64<1>, seteq:ch               : -> 0
 t18: i64 = zero_extend t17                                     : -> 0
...

Optimized lowered selection DAG: %bb.0 'main:entry'
// Combined to load back c as a sign extended i1, which should mean -1 here.
...
t52: ch = store<(store 1 into @c, align 4)> t0, Constant:i1<-1>, ... : i1:1
t51: i64,ch = load<(dereferenceable load 1 from @c, align 4), sext from i1>
                 t9, ... : sext from i1:1         : -1
  t17: i1 = setcc t51, Constant:i64<1>, seteq:ch  : 0
  t18: i64 = zero_extend t17                      : 0
...

Type-legalized selection DAG: %bb.0 'main:entry'
// Storing i32:1, but loading sext from i1, so should still mean -1
...
t55: ch = store<(store 1 into @c, align 4), trunc to i1>
            t0, Constant:i32<1>, ... : 1
t51: i64,ch = load<(dereferenceable load 1 from @c, align 4), sext from i1>
                t9, ...  : -1
 t56: i32 = setcc t51, Constant:i64<1>, seteq:ch   : 0
  t57: i64 = any_extend t56                        : 0
   t58: i64 = and t57, Constant:i64<1>             : -> 0
...

Optimized type-legalized selection DAG: %bb.0 'main:entry'
...
   t56: i32 = setcc t63, Constant:i64<1>, seteq:ch   : 0
  t59: i64 = zero_extend t56                         : 0
t55: ch = store<(store 1 into @c, align 4), trunc to i1>
            t0, Constant:i32<1>, ...
t63: i64,ch = load<(dereferenceable load 1 from @c, align 4), sext from i1>
                t60, ...  : -1
...

Legalized selection DAG: %bb.0 'main:entry'
// Storing 1 as i8 should still work.
// Loading it back as zero-extended from i8 seems wrong (*)
// AssertZext from i1 also seems wrong.
// Now the loaded value is zero-extended to 1, and the comparison returns true
and 1 is selected instead of 0, which is wrong.
...
    t88: i32 = AssertZext t86, ValueType:ch:i1
   t68: i32 = SystemZISD::ICMP t88, Constant:i32<1>,
                TargetConstant:i32<0>  : true
  t72: i32 = SystemZISD::SELECT_CCMASK Constant:i32<1>, Constant:i32<0>,
              TargetConstant:i32<14>, TargetConstant:i32<8>, t68
 t59: i64 = zero_extend t72 : 1
t73: ch = store<(store 1 into @c, align 4), trunc to i8>
            t0, Constant:i32<1>, ...  : 1
t86: i32,ch = load<(dereferenceable load 1 from @c, align 4), zext from i8>
                t60, ...   : 1 ! (*)
...

(*) adjustSubwordCmp() is converting this to a zext load. I suspect this check
to not be sufficient:

 if (uint64_t(SignedValue) + (uint64_t(1) << (NumBits - 1)) > Mask)
    return;

NumBits is 8, but the load is sign-extending from i1.

I tried commenting out adjustSubwordCmp() but the sign extension of the i1 by
the load is still altered and lost. Not sure exactly what is going wrong here...

---

bin/llc -mtriple=s390x-linux-gnu -mcpu=z14  -o - ./main.f.ll -O3

[Quuxplusone]

Attached main.f.ll (2519 bytes, text/plain): reduced test case (main with inlined functions)

[Quuxplusone]

(In reply to Jonas Paulsson from comment #0)

> (*) adjustSubwordCmp() is converting this to a zext load. I suspect this
> check to not be sufficient:
>
>  if (uint64_t(SignedValue) + (uint64_t(1) << (NumBits - 1)) > Mask)
>     return;
>
> NumBits is 8, but the load is sign-extending from i1.
>
> I tried commenting out adjustSubwordCmp() but the sign extension of the i1
> by the load is still altered and lost. Not sure exactly what is going wrong
> here...

So, first of all, you're right that adjustSubwordCmp should not handle i1 loads
at all.  There is never any benefit of using CLI or the like for booleans.

NumBits is 8 because it uses getStoreSizeInBits.  This probably should check
getValueSizeInBits instead (or in addition) to ensure the routine doesn't do
anything for i1 loads.

But of course commenting out should have the same effect, so I guess the next
step would be to figure out what code is now replacing the load instead.  What
I would have expected an i1 load and test to be converted to is a TM.

[Quuxplusone]

Patch at https://reviews.llvm.org/D78187.

[Quuxplusone]

036242b