Skip to content

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
lanza pushed a commit that referenced this pull request Mar 18, 2025
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.
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
Recommit llvm#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 llvm#1100.

After llvm#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
llvm#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.
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