-
Notifications
You must be signed in to change notification settings - Fork 13.6k
tests: debuginfo: Work around or disable broken tests on powerpc #144160
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
f90a6c1
to
9ce00f5
Compare
// Targets with broken f16 cannot link due to missing __gnu_f2h_ieee | ||
// See https://github.com/llvm/llvm-project/issues/97981 | ||
// and https://github.com/rust-lang/compiler-builtins/issues/655 | ||
//@ ignore-powerpc | ||
//@ ignore-powerpc64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that we really should not have types that must be ignored in the same file as other types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So would you suggest splitting off the f16 parts into a f16-globals-metadata
test and disabling that on the affected targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that would be preferable. If it's powerpc, then it's likely going to be others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, pulling f16 into its own test suddenly makes the test pass. I'm not too sure what part of the old vs new code is responsible for LLVM emitting the symbol vs not emitting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, fairly simple it turns out:
This emits __gnu_h2f_ieee
static mut F16: f16 = 1.5;
static mut F32: f16 = 2.5;
fn main() {
_zzz(); // #break
let a = unsafe { (F16, F32) };
}
fn _zzz() {()}
This doesn't
static mut F16: f16 = 1.5;
fn main() {
_zzz(); // #break
let a = unsafe { (F16,) };
}
fn _zzz() {()}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included a workaround instead of disabling the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...huh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my guess is that it tries to convert the f16 to the f32 due to rules about how we represent the tuple layout
f16 support for PowerPC has issues in LLVM, therefore we need a small workaround to prevent LLVM from emitting symbols that don't exist for PowerPC yet. It also appears that unused by-value non-immedate issue with gdb applies to PowerPC targets as well, though I've only tested 64-bit Linux targets. Signed-off-by: Jens Reidel <[email protected]>
9ce00f5
to
1d0eddb
Compare
Signed-off-by: Jens Reidel <[email protected]>
@bors r+ |
f16 support for PowerPC has issues in LLVM, therefore we need a small workaround to prevent LLVM from emitting symbols that don't exist for PowerPC yet.
It also appears that #128973 applies to PowerPC targets as well, though I've only tested 64-bit Linux targets.