-
Notifications
You must be signed in to change notification settings - Fork 101
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
[CIR] [Lowering] [X86_64] Support VAArg for LongDouble #1150
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
Yeah, looks like the stacked PR was incorrectly merged into a user branch instead of main. This looks good after you fix the formatting.
3fcc9a3
to
75c6ac5
Compare
Done |
The test failure looks related, though I don't know why it would only fail on Windows. |
It turns out to be a weird ordering mismatch. In my environment, I see:
where the case come before the const. but the output in the above link is:
but now the const comes before the cast. hmm.. although we can try to use |
Doesn't seem like we have another option? If |
I think it's pretty strange that we're getting different IR output on different machines. We're specifying a target triple, so it should be fully deterministic, right? |
True, if this is exposing non-deterministic behavior, looks like the right opportunity to understand and fix (or at least if we understand and it's more complex an issue can be created and this could be addressed on a follow up PR) |
6e6aae7
to
e039486
Compare
I was afraid to debug with CI. But I added the DAG now. Let's see what happens. |
The CI looks green now. |
Recommit #1101
I am not sure what happened. But that merged PR doesn't show in the git log. Maybe the stacked PR may not get successed? But after all, we need to land it again.
Following off are original commit messages:
This is the following of #1100.
After #1100, when we want to use LongDouble for VAArg, we will be in trouble due to details in X86_64's ABI and this patch tries to address this.
The practical impact the patch is, after this patch, with #1088 and a small following up fix, we can build and run all C's benchmark in SpecCPU 2017. I think it is a milestone.