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

m68k: Can't use bitshift operators (>> and <<) on 64-bit integers #51461

Closed
AnnikaCodes mannequin opened this issue Oct 9, 2021 · 6 comments
Closed

m68k: Can't use bitshift operators (>> and <<) on 64-bit integers #51461

AnnikaCodes mannequin opened this issue Oct 9, 2021 · 6 comments
Labels
backend:m68k bugzilla Issues migrated from bugzilla confirmed Verified by a second party

Comments

@AnnikaCodes
Copy link
Mannequin

AnnikaCodes mannequin commented Oct 9, 2021

Bugzilla Link 52119
Version trunk
OS All
Attachments Diagnostic report when attempting to compile the generated LLVM-IR with llc.
CC @AnnikaCodes,@glaubitz,@tcr,@jrtc27,@mshockwave

Extended Description

LLVM's new m68k target can't handle left-shifts and right-shifts of two 64-bit integers. This bug isn't specific to C or Clang (it can be reproduced with Rust as well), but the following C program is an easy way to reproduce it:

void llvm_bug() {
unsigned long long x = 2;
unsigned long long y = 2;
unsigned long long result = x >> y;
}

It builds fine under an m68k GCC cross-compiler, but under Clang, throws the following error: Cannot select: 0x7fc3c1017300: i32,i32 = srl_parts 0x7fc3c10171c8, 0x7fc3c1017160, 0x7fc3c1017090.

This is the output when building the above program (saved as shift.c) with bin/clang -target m68k-unknown-linux-gnu shift.c -nostdlib:
fatal error: error in backend: Cannot select: 0x7f9644092b00: i32,i32 = srl_parts 0x7f96440929c8, 0x7f9644092960, 0x7f9644092890
0x7f96440929c8: i32,ch = load<(dereferenceable load (s32) from %ir.1 + 4, basealign 8)> 0x7f96440927c0, 0x7f96440926f0, undef:i32
0x7f96440926f0: i32 = or FrameIndex:i32<0>, Constant:i32<4>
0x7f9644091e68: i32 = FrameIndex<0>
0x7f96440924e8: i32 = Constant<4>
0x7f9644091f38: i32 = undef
0x7f9644092960: i32,ch = load<(dereferenceable load (s32) from %ir.1, align 8)> 0x7f96440927c0, FrameIndex:i32<0>, undef:i32
0x7f9644091e68: i32 = FrameIndex<0>
0x7f9644091f38: i32 = undef
0x7f9644092890: i32,ch = load<(dereferenceable load (s32) from %ir.2 + 4, basealign 8)> 0x7f96440927c0, 0x7f96440928f8, undef:i32
0x7f96440928f8: i32 = or FrameIndex:i32<1>, Constant:i32<4>
0x7f9644092008: i32 = FrameIndex<1>
0x7f96440924e8: i32 = Constant<4>
0x7f9644091f38: i32 = undef
In function: llvm_bug
clang-14: error: clang frontend command failed with exit code 70 (use -v to see invocation)
clang version 14.0.0 (https://github.com/llvm/llvm-project.git 9697f93)
Target: m68k-unknown-linux-gnu
Thread model: posix
InstalledDir: /Users/annika/llvm-project/build/bin
clang-14: note: diagnostic msg:


PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang-14: note: diagnostic msg: /var/folders/cn/qwypwgqn3vn7wwn5ksn9lr5w0000gn/T/shift-5ddeee.c
clang-14: note: diagnostic msg: /var/folders/cn/qwypwgqn3vn7wwn5ksn9lr5w0000gn/T/shift-5ddeee.sh
clang-14: note: diagnostic msg: Crash backtrace is located in
clang-14: note: diagnostic msg: /Users/annika/Library/Logs/DiagnosticReports/clang-14__.crash
clang-14: note: diagnostic msg: (choose the .crash file that corresponds to your crash)
clang-14: note: diagnostic msg:


For some reason, clang didn't generate the .crash file it was talking about, but compiling to LLVM-IR and then using llc made the proper diagnostic report, which I have attached.

I understand that m68k support is experimental, but I would still like to report this bug in hopes that it will either eventually be fixed, or someone can help me to fix it myself.

Thanks!

@jrtc27
Copy link
Collaborator

jrtc27 commented Oct 9, 2021

Known bug, was reported the other week on IRC, backend just needs to setOperandAction(ISD::S{HL,RA,RL}_PARTS, MVT::i32, Expand). I thought someone had posted that patch but can't find it so perhaps not. Test case is simply:

define i64 @​lshr64(i64 %a, i64 %b) nounwind {
%1 = lshr i64 %a, %b
ret i64 %1
}

define i64 @​ashr64(i64 %a, i64 %b) nounwind {
%1 = ashr i64 %a, %b
ret i64 %1
}

define i64 @​shl64(i64 %a, i64 %b) nounwind {
%1 = shl i64 %a, %b
ret i64 %1
}

@AnnikaCodes
Copy link
Mannequin Author

AnnikaCodes mannequin commented Oct 9, 2021

Known bug, was reported the other week on IRC, backend just needs to
setOperandAction(ISD::S{HL,RA,RL}_PARTS, MVT::i32, Expand). I thought
someone had posted that patch but can't find it so perhaps not. Test case is
simply:

define i64 @​lshr64(i64 %a, i64 %b) nounwind {
%1 = lshr i64 %a, %b
ret i64 %1
}

define i64 @​ashr64(i64 %a, i64 %b) nounwind {
%1 = ashr i64 %a, %b
ret i64 %1
}

define i64 @​shl64(i64 %a, i64 %b) nounwind {
%1 = shl i64 %a, %b
ret i64 %1
}

Alright, thanks for the info! Should I submit a patch for this?

@glaubitz
Copy link
Contributor

Alright, thanks for the info! Should I submit a patch for this?

If you have a patch fixing the issue, that would be much appreciated. Thank you!

@tcr
Copy link
Mannequin

tcr mannequin commented Nov 3, 2021

Implement SHL_PARTS, SRA_PARTS, and SRL_PARTS for i32
Hi, I was interested in fixing this upstream issue in LLVM M68K backend. Attached is a patch that applies the above recommendation (calling setOperationAction) for the three unimplemented operation types. Let me know if I can refine the patch, thanks.

@glaubitz
Copy link
Contributor

glaubitz commented Nov 3, 2021

Hi Tim!

Hi, I was interested in fixing this upstream issue in LLVM M68K backend.
Attached is a patch that applies the above recommendation (calling
setOperationAction) for the three unimplemented operation types. Let me know
if I can refine the patch, thanks.

There is an ongoing discussion where a patch for this is being discussed:

https://reviews.llvm.org/D111497

Maybe you could join the discussion and help as the current patch introduces a regression.

Thanks!

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
@llvmbot llvmbot added the confirmed Verified by a second party label Jan 26, 2022
@glaubitz
Copy link
Contributor

This was just fixed in 43a1756 and I just verified that by successfully compiling the sample code above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:m68k bugzilla Issues migrated from bugzilla confirmed Verified by a second party
Projects
None yet
Development

No branches or pull requests

4 participants