Skip to content

[InstCombine] Miscompile at -O1 #85536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dtcxzyw opened this issue Mar 16, 2024 · 7 comments · Fixed by #85542
Closed

[InstCombine] Miscompile at -O1 #85536

dtcxzyw opened this issue Mar 16, 2024 · 7 comments · Fixed by #85542

Comments

@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 16, 2024

Reduced test case: https://godbolt.org/z/z1a3665hd

#include <stdio.h>
#include <stdint.h>
static uint16_t
(safe_lshift_func_uint16_t_u_s)(uint16_t left, int right )
{
  return
    ((((int)right) < 0) || (((int)right) >= 32) || (left > ((65535) >> ((int)right)))) ?
    ((left)) :
    (left << ((int)right));
}
static int16_t
(safe_unary_minus_func_int16_t_s)(int16_t si )
{
  return
    -si;
}
static int32_t
(safe_lshift_func_int32_t_s_u)(int32_t left, unsigned int right )
{
  return
    ((left < 0) || (((unsigned int)right) >= 32) || (left > ((2147483647) >> ((unsigned int)right)))) ?
    ((left)) :
    (left << ((unsigned int)right));
}
long smin(long d, long p) { return d < p ? d : p; }
struct e { uint32_t f; } static g[] = {1, 36};
int64_t h, i;
uint8_t j(uint64_t m) {
  if (safe_lshift_func_uint16_t_u_s(
          smin(m, safe_unary_minus_func_int16_t_s(
                                safe_lshift_func_int32_t_s_u(1, g[1].f))),
          3))
     h = 0;
  return m;
}
int8_t k() {
  j(0);
  struct e *l[] = {&g[1], &g[1], &g[1], &g[1], &g[1],
                   &g[1], &g[1], &g[1], &g[1]};
  return i;
}
int main() {
  printf("%d\n", k());
  return 0;
}
> gcc -O0 -fsanitize=address,undefined -w test.c && ./a.out
0
> clang -O3 -w test.c && ./a.out
; no output

Reduced LLVM IR: https://alive2.llvm.org/ce/z/hWuhFG

%struct.e = type { i32 }

@g = internal global [2 x %struct.e] [%struct.e { i32 1 }, %struct.e { i32 36 }], align 4
@h = dso_local local_unnamed_addr global i64 0, align 8
@i = dso_local local_unnamed_addr global i64 0, align 8

define i8 @src() {
entry:
  %0 = load i32, ptr getelementptr inbounds ([2 x %struct.e], ptr @g, i64 0, i64 1), align 4
  %or.cond.i.i = icmp ult i32 %0, 31
  %shl.i.neg.i = shl nsw i32 -1, %0
  %.neg.i = zext i32 %shl.i.neg.i to i64
  %sext.i = shl i64 %.neg.i, 48
  %1 = ashr exact i64 %sext.i, 48
  %2 = call i64 @llvm.smin.i64(i64 %1, i64 0)
  %3 = and i64 %2, 65535
  %tobool.not9.i1 = icmp eq i64 %3, 0
  %tobool.not9.i = and i1 %or.cond.i.i, %tobool.not9.i1
  br i1 %tobool.not9.i, label %j.exit, label %if.then.i

if.then.i:
  store i64 0, ptr @h, align 8
  br label %j.exit

j.exit:
  %4 = load i64, ptr @i, align 8
  %conv = trunc i64 %4 to i8
  ret i8 %conv
}

==>

define i8 @tgt() {
  ret i8 poison
}

LLVM version: 74d1a40

cc @nikic

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 16, 2024

Reduced IR: https://alive2.llvm.org/ce/z/Gv9zkm

@g = internal global [1 x i32] [i32 1], align 4

define i32 @k() {
entry:
  %ret = load i32, ptr getelementptr inbounds ([1 x i32], ptr @g, i64 0), align 4
  ret i32 %ret
}

==>
define i32 @k() {
entry:
  ret i32 1
}

@nikic
Copy link
Contributor

nikic commented Mar 16, 2024

@dtcxzyw That last transform is correct for GlobalOpt, alive2 does not support IPO transforms.

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 16, 2024

@dtcxzyw That last transform is correct for GlobalOpt, alive2 does not support IPO transforms.

Oh, sorry about my mistake. I will provide a godbolt link.

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 16, 2024

The root cause: https://alive2.llvm.org/ce/z/ThJs3Y

define i1 @src(i32 %a) {
entry:
  %or.cond.i.i = icmp ugt i32 %a, 30
  %shl.i.neg.i = shl nsw i32 -1, %a
  %.neg.i = zext i32 %shl.i.neg.i to i64
  %sext.i = shl i64 %.neg.i, 48
  %1 = ashr exact i64 %sext.i, 48
  %conv2.i = select i1 %or.cond.i.i, i64 -1, i64 %1
  %cond.i8.i = call noundef i64 @llvm.smin.i64(i64 0, i64 %conv2.i)
  %2 = and i64 %cond.i8.i, 65535
  %tobool.not9.i = icmp eq i64 %2, 0
  ret i1 %tobool.not9.i
}

==> 

define i1 @tgt(i32 %a) {
entry:
  %or.cond.i.i = icmp ult i32 %a, 31
  %shl.i.neg.i = shl nsw i32 -1, %a
  %.neg.i = zext i32 %shl.i.neg.i to i64
  %sext.i = shl i64 %.neg.i, 48
  %0 = ashr exact i64 %sext.i, 48
  %1 = call noundef i64 @llvm.smin.i64(i64 %0, i64 0)
  %2 = and i64 %1, 65535
  %tobool.not9.i1 = icmp eq i64 %2, 0
  %tobool.not9.i = and i1 %or.cond.i.i, %tobool.not9.i1
  ret i1 %tobool.not9.i
}

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 16, 2024

We forget to drop the noundef attribute for smin :(

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 16, 2024

Minimal reproducer: https://alive2.llvm.org/ce/z/aBubZ4

define i8 @src(i1 %cond, i8 %val) {
entry:
  %sel = select i1 %cond, i8 -1, i8 %val
  %ret = call noundef i8 @llvm.smin.i8(i8 %sel, i8 0)
  ret i8 %ret
}

==>

define i8 @tgt(i1 %cond, i8 %val) {
  %min = call noundef i8 @llvm.smin.i8(i8 %val, i8 0)
  %ret = select i1 %cond, i8 -1, i8 %min
  ret i8 %ret
}

@nikic
Copy link
Contributor

nikic commented Mar 16, 2024

@dtcxzyw We're probably missing a call to dropUBImplyingAttrsAndMetadata in FoldOpIntoSelect.

dtcxzyw added a commit that referenced this issue Mar 17, 2024
…struction (#85542)

When speculating an instruction in `InstCombinerImpl::FoldOpIntoSelect`,
the call may result in undefined behavior. This patch drops all
UB-implying attrs/metadata to fix this.

Fixes #85536.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Mar 17, 2024
…struction (llvm#85542)

When speculating an instruction in `InstCombinerImpl::FoldOpIntoSelect`,
the call may result in undefined behavior. This patch drops all
UB-implying attrs/metadata to fix this.

Fixes llvm#85536.

(cherry picked from commit 252d019)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants