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

[RISCV] Miscompile at -O3 with zbb #85190

Open
dtcxzyw opened this issue Mar 14, 2024 · 5 comments · May be fixed by #84924
Open

[RISCV] Miscompile at -O3 with zbb #85190

dtcxzyw opened this issue Mar 14, 2024 · 5 comments · May be fixed by #84924

Comments

@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 14, 2024

Reduced test case:

#include <stdint.h>
#include <stdio.h>
int8_t d;
int32_t b;
uint64_t c;
int64_t safe_add_func_int64_t_s_s(int64_t si1, int64_t si2)
{
  return ((si1<0) && (si2<0) && (si1 < (INT64_MIN-si2))) ?
    (si1) :
    (si1 + si2);
}
int main() {
  do b = safe_add_func_int64_t_s_s(c, 7 | c);
  while (d);
  printf("%d\n", b);
  return 0;
}
> gcc -O0 test.c -fsanitize=undefined,address && ./a.out
7
> bin/clang -O3 -march=rv64gc_zbb test.c --target=riscv64-linux-gnu
> qemu-riscv64 -L /usr/riscv64-linux-gnu -cpu rv64,zbb=true ./a.out
0

LLVM version: eb21ee4
qemu version: v8.2.1

cc @topperc @asb

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 14, 2024

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

Author: Yingwei Zheng (dtcxzyw)

Reduced test case: ``` #include <stdint.h> #include <stdio.h> int8_t d; int32_t b; uint64_t c; int64_t safe_add_func_int64_t_s_s(int64_t si1, int64_t si2) { return ((si1<0) && (si2<0) && (si1 < (INT64_MIN-si2))) ? (si1) : (si1 + si2); } int main() { do b = safe_add_func_int64_t_s_s(c, 7 | c); while (d); printf("%d\n", b); return 0; } ```
&gt; gcc -O0 test.c -fsanitize=undefined,address &amp;&amp; ./a.out
7
&gt; bin/clang -O3 -march=rv64gc_zbb test.c --target=riscv64-linux-gnu
&gt; qemu-riscv64 -L /usr/riscv64-linux-gnu -cpu rv64,zbb=true ./a.out
0

cc @topperc @asb

@dtcxzyw dtcxzyw self-assigned this Mar 14, 2024
@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 14, 2024

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 14, 2024

Reduced IR: https://godbolt.org/z/f8Th3dhPs

define i1 @test1(i64 %a) {
  %or = or i64 %a, 7
  %or.cond.not.i = icmp slt i64 %a, 0
  %sub.i = sub nsw i64 -9223372036854775808, %or
  %cmp3.i = icmp sgt i64 %sub.i, %a
  %or.cond.i = select i1 %or.cond.not.i, i1 %cmp3.i, i1 false
  ret i1 %or.cond.i
}

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 14, 2024

Alive2: https://alive2.llvm.org/ce/z/4YC2nc

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 14, 2024

Should be fixed by #84924.

bjope added a commit to bjope/llvm-project that referenced this issue Mar 15, 2024
bjope added a commit to bjope/llvm-project that referenced this issue Mar 15, 2024
Just like for regular IR we need to treat SELECT as conditionally
blocking poison. So (unless the condition itself is poison) the
result is only poison if the selected true/false value is poison.
Thus, when doing DAG combines that turn SELECT into arithmetic/logical
operations (e.g. AND/OR) we need to make sure that the new operations
aren't more poisonous. One way to do that is to use FREEZE to make
sure the operands aren't posion.

This patch aims at fixing the kind of miscompiles reported in
  llvm#84653
and
  llvm#85190

Solution is to make sure that we insert FREEZE, if needed to make
the fold sound, when using the foldBoolSelectToLogic and
foldVSelectToSignBitSplatMask DAG combines.

This may result in some (hopefully minor) regressions since we lack
some ways to fold away the freeze (or due to isGuaranteedNotToBePoison
being too pessimistic). Focus in this patch is to just avoid
miscompiles, but I think some of the regressions can be avoided by
general improvements regarding poison/freeze handling in SelectionDAG.
bjope added a commit to bjope/llvm-project that referenced this issue Apr 9, 2024
bjope added a commit to bjope/llvm-project that referenced this issue Apr 9, 2024
Just like for regular IR we need to treat SELECT as conditionally
blocking poison. So (unless the condition itself is poison) the
result is only poison if the selected true/false value is poison.
Thus, when doing DAG combines that turn SELECT into arithmetic/logical
operations (e.g. AND/OR) we need to make sure that the new operations
aren't more poisonous. One way to do that is to use FREEZE to make
sure the operands aren't posion.

This patch aims at fixing the kind of miscompiles reported in
  llvm#84653
and
  llvm#85190

Solution is to make sure that we insert FREEZE, if needed to make
the fold sound, when using the foldBoolSelectToLogic and
foldVSelectToSignBitSplatMask DAG combines.

This may result in some (hopefully minor) regressions since we lack
some ways to fold away the freeze (or due to isGuaranteedNotToBePoison
being too pessimistic). Focus in this patch is to just avoid
miscompiles, but I think some of the regressions can be avoided by
general improvements regarding poison/freeze handling in SelectionDAG.
bjope added a commit that referenced this issue Apr 26, 2024
bjope added a commit to bjope/llvm-project that referenced this issue Apr 26, 2024
Just like for regular IR we need to treat SELECT as conditionally
blocking poison. So (unless the condition itself is poison) the
result is only poison if the selected true/false value is poison.
Thus, when doing DAG combines that turn SELECT into arithmetic/logical
operations (e.g. AND/OR) we need to make sure that the new operations
aren't more poisonous. One way to do that is to use FREEZE to make
sure the operands aren't posion.

This patch aims at fixing the kind of miscompiles reported in
  llvm#84653
and
  llvm#85190

Solution is to make sure that we insert FREEZE, if needed to make
the fold sound, when using the foldBoolSelectToLogic and
foldVSelectToSignBitSplatMask DAG combines.

This may result in some (hopefully minor) regressions since we lack
some ways to fold away the freeze (or due to isGuaranteedNotToBePoison
being too pessimistic). Focus in this patch is to just avoid
miscompiles, but I think some of the regressions can be avoided by
general improvements regarding poison/freeze handling in SelectionDAG.
@dtcxzyw dtcxzyw removed their assignment May 14, 2024
bjope added a commit to bjope/llvm-project that referenced this issue May 28, 2024
Just like for regular IR we need to treat SELECT as conditionally
blocking poison. So (unless the condition itself is poison) the
result is only poison if the selected true/false value is poison.
Thus, when doing DAG combines that turn SELECT into arithmetic/logical
operations (e.g. AND/OR) we need to make sure that the new operations
aren't more poisonous. One way to do that is to use FREEZE to make
sure the operands aren't posion.

This patch aims at fixing the kind of miscompiles reported in
  llvm#84653
and
  llvm#85190

Solution is to make sure that we insert FREEZE, if needed to make
the fold sound, when using the foldBoolSelectToLogic and
foldVSelectToSignBitSplatMask DAG combines.

This may result in some (hopefully minor) regressions since we lack
some ways to fold away the freeze (or due to isGuaranteedNotToBePoison
being too pessimistic). Focus in this patch is to just avoid
miscompiles, but I think some of the regressions can be avoided by
general improvements regarding poison/freeze handling in SelectionDAG.
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