Skip to content
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

[RISC-V] Miscompile at -O2 #83947

Closed
patrick-rivos opened this issue Mar 5, 2024 · 4 comments · Fixed by #83993
Closed

[RISC-V] Miscompile at -O2 #83947

patrick-rivos opened this issue Mar 5, 2024 · 4 comments · Fixed by #83993

Comments

@patrick-rivos
Copy link
Contributor

Testcase:

int printf(const char *, ...);
int a, c = 1;
int b[2];
int main() {
d:
  for (; a < 8; a += 1) {
    int *e = &c;
    if (&b[1] == e)
      *e = 0;
  }
f:
  if (0) {
    goto f;
    goto d;
  }
  printf("%d\n", c);
}

The condition if (&b[1] == e) should never be true.

Commands:

> /scratch/tc-testing/tc-mar-4-llvm/build/bin/clang -march=rv64gcv -O2 red.c -o red.out -fno-strict-aliasing
> /scratch/tc-testing/tc-mar-4-llvm/build/bin/qemu-riscv64 red.out
0
> /scratch/tc-testing/tc-mar-4-llvm/build/bin/clang red.c -o red.out -fno-strict-aliasing
> /scratch/tc-testing/tc-mar-4-llvm/build/bin/qemu-riscv64 red.out
1

Godbolt: https://godbolt.org/z/eEfqjf5rr

-opt-bisect-limit points at InstCombinePass

Discovered/tested using version 57592e9 (not bisected)

Found using fuzzer

@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 5, 2024

Confirmed. But it looks like a RISC-V backend bug.
cc @wangpc-pp @topperc @lukel97

@llvmbot
Copy link
Member

llvmbot commented Mar 5, 2024

@llvm/issue-subscribers-backend-risc-v

Author: Patrick O'Neill (patrick-rivos)

Testcase: ```c int printf(const char *, ...); int a, c = 1; int b[2]; int main() { d: for (; a < 8; a += 1) { int *e = &c; if (&b[1] == e) *e = 0; } f: if (0) { goto f; goto d; } printf("%d\n", c); } ``` The condition `if (&b[1] == e)` should never be true.

Commands:

&gt; /scratch/tc-testing/tc-mar-4-llvm/build/bin/clang -march=rv64gcv -O2 red.c -o red.out -fno-strict-aliasing
&gt; /scratch/tc-testing/tc-mar-4-llvm/build/bin/qemu-riscv64 red.out
0
&gt; /scratch/tc-testing/tc-mar-4-llvm/build/bin/clang red.c -o red.out -fno-strict-aliasing
&gt; /scratch/tc-testing/tc-mar-4-llvm/build/bin/qemu-riscv64 red.out
1

Godbolt: https://godbolt.org/z/eEfqjf5rr

-opt-bisect-limit points at InstCombinePass

Discovered/tested using version 57592e9 (not bisected)

Found using fuzzer

@topperc
Copy link
Collaborator

topperc commented Mar 5, 2024

Confirmed. But it looks like a RISC-V backend bug. cc @wangpc-pp @topperc @lukel97

The bug is in InstCombine

This transforms assumes the mask is a non-zero splat. We only know its a splat and not provably all 0s. The mask is a constexpr that includes the address of the global variable. We can't resolve the constant expression to an exact value.

    // scatter(splat(value), splat(ptr), non-zero-mask) -> store value, ptr
    if (auto *SplatValue = getSplatValue(II.getArgOperand(0))) {
      Align Alignment = cast<ConstantInt>(II.getArgOperand(2))->getAlignValue(); 
      StoreInst *S =
          new StoreInst(SplatValue, SplatPtr, /*IsVolatile=*/false, Alignment);  
      S->copyMetadata(II);
      return S;
    }

@topperc
Copy link
Collaborator

topperc commented Mar 5, 2024

We need to loop through all elements of the mask and check if any of them are a ConstantInt of 1. Not sure if there's a routine for that already.

@dtcxzyw dtcxzyw self-assigned this Mar 5, 2024
dtcxzyw added a commit that referenced this issue Mar 5, 2024
https://github.com/llvm/llvm-project/blob/762f762504967efbe159db5c737154b989afc9bb/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L394-L407

Comment from @topperc:
> This transforms assumes the mask is a non-zero splat. We only know its
a splat and not provably all 0s. The mask is a constexpr that includes
the address of the global variable. We can't resolve the constant
expression to an exact value.

Fixes #83947.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Mar 5, 2024
https://github.com/llvm/llvm-project/blob/762f762504967efbe159db5c737154b989afc9bb/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L394-L407

Comment from @topperc:
> This transforms assumes the mask is a non-zero splat. We only know its
a splat and not provably all 0s. The mask is a constexpr that includes
the address of the global variable. We can't resolve the constant
expression to an exact value.

Fixes llvm#83947.

(cherry picked from commit a1a590e)
dtcxzyw added a commit to dtcxzyw/llvm-project that referenced this issue Mar 7, 2024
https://github.com/llvm/llvm-project/blob/762f762504967efbe159db5c737154b989afc9bb/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L394-L407

Comment from @topperc:
> This transforms assumes the mask is a non-zero splat. We only know its
a splat and not provably all 0s. The mask is a constexpr that includes
the address of the global variable. We can't resolve the constant
expression to an exact value.

Fixes llvm#83947.
tstellar pushed a commit to dtcxzyw/llvm-project that referenced this issue Mar 13, 2024
https://github.com/llvm/llvm-project/blob/762f762504967efbe159db5c737154b989afc9bb/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L394-L407

Comment from @topperc:
> This transforms assumes the mask is a non-zero splat. We only know its
a splat and not provably all 0s. The mask is a constexpr that includes
the address of the global variable. We can't resolve the constant
expression to an exact value.

Fixes llvm#83947.
xgupta pushed a commit to xgupta/llvm-project that referenced this issue Sep 9, 2024
https://github.com/llvm/llvm-project/blob/762f762504967efbe159db5c737154b989afc9bb/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L394-L407

Comment from @topperc:
> This transforms assumes the mask is a non-zero splat. We only know its
a splat and not provably all 0s. The mask is a constexpr that includes
the address of the global variable. We can't resolve the constant
expression to an exact value.

Fixes llvm#83947.
xgupta pushed a commit to xgupta/llvm-project that referenced this issue Oct 10, 2024
https://github.com/llvm/llvm-project/blob/762f762504967efbe159db5c737154b989afc9bb/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L394-L407

Comment from @topperc:
> This transforms assumes the mask is a non-zero splat. We only know its
a splat and not provably all 0s. The mask is a constexpr that includes
the address of the global variable. We can't resolve the constant
expression to an exact value.

Fixes llvm#83947.
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.

5 participants