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

fshl-related miscompile by arm64 and x86-64 backends #55296

Closed
regehr opened this issue May 6, 2022 · 6 comments
Closed

fshl-related miscompile by arm64 and x86-64 backends #55296

regehr opened this issue May 6, 2022 · 6 comments

Comments

@regehr
Copy link
Contributor

regehr commented May 6, 2022

ok this one is a bit of a pain so please bear with me:

declare i7 @llvm.fshl.i7(i7, i7, i7) #0

define i7 @f(i7 %0, i7 %1, i6 %2) {
  %4 = sext i6 %2 to i7
  %5 = call i7 @llvm.fshl.i7(i7 %0, i7 %1, i7 %4)
  ret i7 %5
}

let's play through what this code should do when called as f(8, 0, 39)

%0 = 0b0001000
%1 = 0b0000000
%2 = 0b100111
%4 = 0b1100111

the concatenation of %0 and %1 is 0b00010000000000 and the shift amount is 103 mod 7, or 5. thus, after the shift we get a run of all zeroes, therefore f(8, 0, 39) should be 0.

Alive2 agrees with this analysis: https://alive2.llvm.org/ce/z/Ph-9vE

but this isn't what we get from either the x64 or arm64 backend. let's use this driver:

#include <stdio.h>

// define i7 @f(i7 %0, i7 %1, i6 %2) {
unsigned f(unsigned, unsigned, unsigned);

int main(void) {
  printf("%u\n", f(8, 0, 39));
  return 0;
}

on x64 we get:

regehr@john-home:~$ llc foo.ll && clang foo.c foo.s && ./a.out
8
regehr@john-home:~$ 

and on arm64:

Johns-MacBook-Pro:~ regehr$ ~/llvm-project/for-alive/bin/llc foo.ll && clang foo.c foo.s && ./a.out 
8
Johns-MacBook-Pro:~ regehr$

cc @ornata @nunoplopes @ryan-berger @nbushehri @zhengyang92 @aqjune @Hatsunespica

@llvmbot
Copy link
Collaborator

llvmbot commented May 6, 2022

@llvm/issue-subscribers-backend-aarch64

@llvmbot
Copy link
Collaborator

llvmbot commented May 6, 2022

@llvm/issue-subscribers-backend-x86

@regehr
Copy link
Contributor Author

regehr commented May 6, 2022

(sorry, I'm have a million alive2 tabs open, I think this is the correct one):
https://alive2.llvm.org/ce/z/3pMSgB

@topperc
Copy link
Collaborator

topperc commented May 6, 2022

I think this might be the fix

diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
index a1ddb02563e3..ba76a4696146 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
@@ -1277,7 +1277,7 @@ SDValue DAGTypeLegalizer::PromoteIntRes_Rotate(SDNode *N) {
 SDValue DAGTypeLegalizer::PromoteIntRes_FunnelShift(SDNode *N) {
   SDValue Hi = GetPromotedInteger(N->getOperand(0));
   SDValue Lo = GetPromotedInteger(N->getOperand(1));
-  SDValue Amt = GetPromotedInteger(N->getOperand(2));
+  SDValue Amt = ZExtPromotedInteger(N->getOperand(2));

   SDLoc DL(N);
   EVT OldVT = N->getOperand(0).getValueType();

We need to make sure we clear any promoted bits before creating the UREM node.

@topperc
Copy link
Collaborator

topperc commented May 6, 2022

Candidate patch https://reviews.llvm.org/D125076

@topperc
Copy link
Collaborator

topperc commented May 6, 2022

Should be fixed after 76f90a9

@topperc topperc closed this as completed May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants