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

[CIR] [Lowering] [X86_64] Support VAArg for LongDouble #1150

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

ChuanqiXu9
Copy link
Member

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.

Copy link

github-actions bot commented Nov 21, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@smeenai smeenai left a 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.

@ChuanqiXu9
Copy link
Member Author

Yeah, looks like the stacked PR was incorrectly merged into a user branch instead of main. This looks good after you fix the formatting.

Done

@smeenai
Copy link
Collaborator

smeenai commented Nov 21, 2024

The test failure looks related, though I don't know why it would only fail on Windows.

@ChuanqiXu9
Copy link
Member Author

It turns out to be a weird ordering mismatch. In my environment, I see:

cir.va.start %4 : !cir.ptr<!ty___va_list_tag> loc(#loc23)
    %5 = cir.cast(array_to_ptrdecay, %2 : !cir.ptr<!cir.array<!ty___va_list_tag x 1>>), !cir.ptr<!ty___va_list_tag> loc(#loc21)
    %6 = cir.get_member %5[2] {name = "overflow_arg_area"} : !cir.ptr<!ty___va_list_tag> -> !cir.ptr<!cir.ptr<!void>> loc(#loc21)
    %7 = cir.load %6 : !cir.ptr<!cir.ptr<!void>>, !cir.ptr<!void> loc(#loc21)
    %8 = cir.cast(bitcast, %7 : !cir.ptr<!void>), !cir.ptr<!u8i> loc(#loc21)
    %9 = cir.const #cir.int<15> : !u32i loc(#loc21)
    %10 = cir.ptr_stride(%8 : !cir.ptr<!u8i>, %9 : !u32i), !cir.ptr<!u8i> loc(#loc21)

where the case come before the const.

but the output in the above link is:

cir.va.start %4 : !cir.ptr<!ty___va_list_tag> loc(#loc23) 
# |             66:  %5 = cir.cast(array_to_ptrdecay, %2 : !cir.ptr<!cir.array<!ty___va_list_tag x 1>>), !cir.ptr<!ty___va_list_tag> loc(#loc21) 
# |             67:  %6 = cir.get_member %5[2] {name = "overflow_arg_area"} : !cir.ptr<!ty___va_list_tag> -> !cir.ptr<!cir.ptr<!void>> loc(#loc21) 
# |             68:  %7 = cir.load %6 : !cir.ptr<!cir.ptr<!void>>, !cir.ptr<!void> loc(#loc21) 
# |             69:  %8 = cir.const #cir.int<15> : !u32i loc(#loc21) 
# |             70:  %9 = cir.cast(bitcast, %7 : !cir.ptr<!void>), !cir.ptr<!u8i> loc(#loc21) 
# | check:122'0                                                  X~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
# |             71:  %10 = cir.ptr_stride(%9 : !cir.ptr<!u8i>, %8 : !u32i), !cir.ptr<!u8i> loc(#loc21) 

but now the const comes before the cast.

hmm.. although we can try to use DAG to check it. But I prefer to not debugging with CI (I don't have windows environment). So I prefer to skip this test on windows.

@bcardosolopes
Copy link
Member

But I prefer to not debugging with CI (I don't have windows environment). So I prefer to skip this test on windows.

Doesn't seem like we have another option? If DAG works why not use it? Disabling stuff for other platforms will create tech debt for no good reason here

@smeenai
Copy link
Collaborator

smeenai commented Nov 22, 2024

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?

@bcardosolopes
Copy link
Member

I think it's pretty strange that we're getting different IR output on different machines

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)

@ChuanqiXu9
Copy link
Member Author

But I prefer to not debugging with CI (I don't have windows environment). So I prefer to skip this test on windows.

Doesn't seem like we have another option? If DAG works why not use it? Disabling stuff for other platforms will create tech debt for no good reason here

I was afraid to debug with CI. But I added the DAG now. Let's see what happens.

@ChuanqiXu9
Copy link
Member Author

The CI looks green now.

@bcardosolopes bcardosolopes merged commit 2a95651 into llvm:main Nov 25, 2024
6 checks passed
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.

3 participants