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

handle missing op freeze #186

Closed
wants to merge 1 commit into from
Closed

Conversation

yxd-ym
Copy link
Contributor

@yxd-ym yxd-ym commented Sep 8, 2024

it seems that freeze is missing from PromoteFloatResult

@yxd-ym yxd-ym mentioned this pull request Sep 8, 2024
@alexrp
Copy link
Member

alexrp commented Sep 8, 2024

Do you have some LLVM IR that reproduces the issue?

@nektro
Copy link

nektro commented Sep 8, 2024

patches here need to be sent to https://github.com/llvm/llvm-project or https://github.com/ziglang/zig and then pulled back in

@andrewrk andrewrk closed this Sep 8, 2024
@andrewrk
Copy link
Member

andrewrk commented Sep 8, 2024

This project does not carry functional patches from LLVM, only patches to help get it to build from source. You have to work with LLVM upstream if you want this change.

@yxd-ym
Copy link
Contributor Author

yxd-ym commented Sep 8, 2024

Do you have some LLVM IR that reproduces the issue?

No, I don't.

I checked the error message Do not know how to promote this operator's result! and found the following code piece

switch (N->getOpcode()) {
// These opcodes cannot appear if promotion of FP16 is done in the backend
// instead of Clang
case ISD::FP16_TO_FP:
case ISD::FP_TO_FP16:
default:
#ifndef NDEBUG
dbgs() << "PromoteFloatResult #" << ResNo << ": ";
N->dump(&DAG); dbgs() << "\n";
#endif
report_fatal_error("Do not know how to promote this operator's result!");

I found this OP by changing the error log message to output the OP name because the default branch is really suspicious.

And when bootstrapping loongarch64-linux-musl target again, the error message outputs that the OP that causing this error is FREEZE.

I think this OP is generated by compiling some code of zig (maybe musl?). But I've no idea which piece of code makes the compiler generates FREEZE op.

@yxd-ym
Copy link
Contributor Author

yxd-ym commented Sep 8, 2024

This project does not carry functional patches from LLVM, only patches to help get it to build from source. You have to work with LLVM upstream if you want this change.

OK, got it.

@yxd-ym yxd-ym deleted the llvm19-fix-loongarch branch September 8, 2024 07:26
@yxd-ym
Copy link
Contributor Author

yxd-ym commented Sep 8, 2024

Do you have some LLVM IR that reproduces the issue?

@alexrp

OK, I've got some from this commit: llvm/llvm-project@0019c2f

define half @freeze_half() nounwind {
  %y1 = freeze half undef
  %t1 = fadd half %y1, %y1
  ret half %t1
}

@alexrp
Copy link
Member

alexrp commented Sep 8, 2024

Seems to be a good repro: https://godbolt.org/z/PPfvWjjG5

I think your proposed fix is reasonable, especially considering that commit. I'm familiar with the LLVM contribution process now, so let me know if you'd like any help submitting the fix upstream. 🙂

@yxd-ym
Copy link
Contributor Author

yxd-ym commented Sep 9, 2024

I created a PR to llvm llvm/llvm-project#107791

@andrewrk
Copy link
Member

andrewrk commented Sep 9, 2024

Nice work. If it is merged upstream we can carry the patch

@yxd-ym
Copy link
Contributor Author

yxd-ym commented Sep 18, 2024

I created a PR to llvm llvm/llvm-project#107791

@alexrp This PR is merged into main. However, I've no idea how to get it released in 19.x. Since llvm 19.1.0 is already released, maybe we need to figure it out how to get it into 19.1.1 (which is 2 weeks later?).

@alexrp
Copy link
Member

alexrp commented Sep 18, 2024

I think you would need to ask the LLVM LoongArch maintainers (ping them on the PR) if they think it's worth backporting for the next patch release.

But if they prefer not to backport it for some reason, then as Andrew said, we can apply the patch to this repo until LLVM 20 since it has been merged upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants