[CIR][LowerToLLVM][CXXABI] Lower cir.va.arg #573
Conversation
clang/lib/CIR/Dialect/Transforms/LoweringPrepareArm64CXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareArm64CXXABI.cpp
Outdated
Show resolved
Hide resolved
ghehg
force-pushed
the
main
branch
3 times, most recently
from
168ca49
to
0a6e761
Compare
bcardosolopes
changed the title
| @@ -1,4 +1,5 @@ | |||
| //====- LoweringPrepareItaniumCXXABI.h - Itanium ABI specific code --------===// | |||
| //====- LoweringPrepareItaniumCXXABI.cpp - Itanium ABI specific code | |||
| //--------===// | |||
There was a problem hiding this comment.
ops, this should fit in 80 instead
There was a problem hiding this comment.
Not resolved, please address this.
clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareAArch64CXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareAArch64CXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareAArch64CXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareAArch64CXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareAArch64CXXABI.cpp
Outdated
Show resolved
Hide resolved
ghehg
force-pushed
the
main
branch
2 times, most recently
from
3adfff7
to
acede3e
Compare
| @@ -1,4 +1,5 @@ | |||
| //====- LoweringPrepareItaniumCXXABI.h - Itanium ABI specific code --------===// | |||
| //====- LoweringPrepareItaniumCXXABI.cpp - Itanium ABI specific code | |||
| //--------===// | |||
There was a problem hiding this comment.
Not resolved, please address this.
clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareAArch64CXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareAArch64CXXABI.cpp
Outdated
Show resolved
Hide resolved
clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.h
Outdated
Show resolved
Hide resolved
clang/test/CIR/CodeGen/variadics.c
Outdated
|
|
||
| va_copy(args_copy, args); | ||
| // CHECK: cir.va.copy %{{[0-9]+}} to %{{[0-9]+}} : !cir.ptr<[[VALISTTYPE]]>, !cir.ptr<[[VALISTTYPE]]> | ||
|
|
||
| // ARM64_CHECK: cir.va.copy %{{[0-9]+}} to %{{[0-9]+}} : !cir.ptr<[[VALISTTYPE]]>, !cir.ptr<[[VALISTTYPE]]> |
There was a problem hiding this comment.
Previous review I gave you an outline of how you should deal with the tests, but seems none was applied?
| auto castRegTop = builder.createBitcast(regTop, i8PtrTy); | ||
| // On big-endian platforms, the value will be right-aligned in its stack slot. | ||
| // and we also need to think about other ABI lowering concerns listed below. | ||
| assert(!cir::MissingFeatures::handleBigEndian()); |
There was a problem hiding this comment.
We should go a bit further here. The skeleton is really important:
// TODO(cir): implement check for jomogeneous aggregate
assert(!cir::MissingFeatures::supportisHomogeneousAggregateQueryForAArch64());
...
uint64_t NumMembers = <expected-default>...
bool IsHFA = <expected-default>...
if (IsHFA && NumMembers > 1) {
... // or unrecheable
} else {
... // or unrecheable
}
For the path that it's not unrecheable, add more skeleton and perhaps more unrecheable for other inner/sub checks that are not implemented/tested.
| // which are all ABI defined thus need ABI lowering query system. | ||
| // The implementation we have now supports most common cases which assumes | ||
| // no indirectness, no alignment greater than 8, and little endian. | ||
| assert(!cir::MissingFeatures::handleBigEndian()); |
There was a problem hiding this comment.
I rather see the big endian check in the place where it happens in the original code (this should be applied elsewhere in this function where big endian is queried), skeleton is important! Also, CIRDataLayout does support big endian queriers, so you can query it and assert on the leg that's not implemented.
There was a problem hiding this comment.
lib/CIR/CodeGen/CIRDataLayout.h:22:class CIRDataLayout
The problem (again) is CIRDataLayout is declared and defined in CodeGen dir.
So I just added another missing feature.
There was a problem hiding this comment.
Oh right, as part of LoweringPrepare we gonna have to keep adding these bits as we go. Sorry for the detour, but please create another PR that moves CIRDataLayout to include/clang/CIR/Dialect/IR/, should be straight fwd. We land that and you rebase this one on top so you can use it here.
lowering var_arg op for ARM64 architecture. This is CIR lowering.
This PR modified LoweringPrepare CXXABI code to make LoweringPrepareArm64CXXABI class inherit more generic LoweringPrepareItaniumCXXABI, this way lowering var_arg would be only meaningful for arm64 targets and for other arch its no op for now.
The ABI doc and detailed algorithm description can be found in this official doc.
The following pic is a easier-to-understand and close to lowered code explanation