[clang] [clang][CodeGen] Zero init unspecified fields in initializers in C (PR #97121)
@@ -361,6 +368,13 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const
VarDecl ,
}
return GV;
}
+ if (!getLangOpts().CPlusPlus) {
+// In C, when an initializer is given, the Linux kernel relies on clang to
+// zero-initialize all members not explicitly initialized, including
padding
+// bits.
rjmccall wrote:
> The standard is ambiguous on this, so can't be used as a reason for the
> change. Please let me know if you have better idea about how to reword the
> comment.
I suggest quoting the standard, identifying the ambiguity, and then saying that
we've chosen to interpret it as requiring all padding in the object to be
zeroed. Since there are multiple paths in the compiler that need this logic,
you may want to write this in a single place and then cross-reference it from
other places.
> I agree. But when I was trying to do it before, I need to fix a lot more
> tests. And the Linux kernel only has C code. I don't have strong reason to
> push this to C++. So I want to limit the change to C at first. If reviewers
> think we should also do it in C++, shall I do it in a follow up change?
I don't mind staging it in over multiple commits in principle. If you'd like
to switch to an implementation that adds explicit padding fields when building
the original constant, you *could* still stage that and only add those fields
in certain language modes.
> No, the non-constant initializer goes through the same implemented path as
> "variables assigned by Compound literals after variable definition". I guess
> I'd better do it in the same patch. Do you have any suggestions on how to
> implement it?
We probably do need to consider it when deciding whether to memset, but naively
I'd expect that you'd just emit little memsets for each individual piece of
padding when we recognize that there's a gap between successive fields. Does
that not work?
https://github.com/llvm/llvm-project/pull/97121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Zero init unspecified fields in initializers in C (PR #97121)
@@ -361,6 +368,13 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const
VarDecl ,
}
return GV;
}
+ if (!getLangOpts().CPlusPlus) {
+// In C, when an initializer is given, the Linux kernel relies on clang to
+// zero-initialize all members not explicitly initialized, including
padding
+// bits.
rjmccall wrote:
> > But I assume that similar code is required in order to explicitly zero out
> > any padding when dynamically initializing a local variable.
>
> I guess you mean variables assigned by Compound literals after variable
> definition. An example is in https://godbolt.org/z/caaEKnjWe.
No, I mean initializing a local aggregate with a non-constant initializer, e.g.
```
void g(char c) {
struct A a = { c, 2 };
}
```
Do we already zero the padding correctly in this case?
https://github.com/llvm/llvm-project/pull/97121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Clang] Fix definition of layout-compatible to ignore empty classes (PR #92103)
@@ -160,6 +160,9 @@ Bug Fixes in This Version Bug Fixes to Compiler Builtins ^^ +- ``__is_layout_compatible`` no longer requires the empty bases to be the same in two + standard-layout classes. It now only compares non-static data members. rjmccall wrote: Is the plan to remove the release note from LLVM 20 if we're able to backport the fix? https://github.com/llvm/llvm-project/pull/92103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Clang] Fix definition of layout-compatible to ignore empty classes (PR #92103)
@@ -160,6 +160,9 @@ Bug Fixes in This Version Bug Fixes to Compiler Builtins ^^ +- ``__is_layout_compatible`` no longer requires the empty bases to be the same in two + standard-layout classes. It now only compares non-static data members. rjmccall wrote: Hmm. I was under the impression that this feature had not been previously released; is that incorrect? https://github.com/llvm/llvm-project/pull/92103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Clang] Fix definition of layout-compatible to ignore empty classes (PR #92103)
https://github.com/rjmccall commented: The code changes look right to me. https://github.com/llvm/llvm-project/pull/92103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Clang] Fix definition of layout-compatible to ignore empty classes (PR #92103)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/92103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] The linux kernel expects initializers to FULLY initialize variables, structs and unions including padding (PR #97121)
@@ -361,6 +368,13 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const
VarDecl ,
}
return GV;
}
+ if (!getLangOpts().CPlusPlus) {
+// In C, when an initializer is given, the Linux kernel relies on clang to
+// zero-initialize all members not explicitly initialized, including
padding
+// bits.
rjmccall wrote:
Also, I think we should do this unconditionally in all language modes. It's
not like this is forbidden in C++.
https://github.com/llvm/llvm-project/pull/97121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] The linux kernel expects initializers to FULLY initialize variables, structs and unions including padding (PR #97121)
@@ -361,6 +368,13 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const
VarDecl ,
}
return GV;
}
+ if (!getLangOpts().CPlusPlus) {
+// In C, when an initializer is given, the Linux kernel relies on clang to
+// zero-initialize all members not explicitly initialized, including
padding
+// bits.
rjmccall wrote:
Comments in the compiler generally shouldn't refer to specific projects. We
are making a guarantee based on our interpretation of the standard.
Similarly, the PR title is also inappropriate. It is fine to note in the PR
description that Linux relies on this for correctness, but the PR title should
describe the semantic change.
https://github.com/llvm/llvm-project/pull/97121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] The linux kernel expects initializers to FULLY initialize variables, structs and unions including padding (PR #97121)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/97121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] The linux kernel expects initializers to FULLY initialize variables, structs and unions including padding (PR #97121)
https://github.com/rjmccall commented: Is this really all that's required? It looks like you're just filling in explicit zero padding when emitting constant initializers. That should steer clear of any possibility that LLVM would treat the padding as `undef` for optimization purposes (surely `undef` bytes are still emitted as zero when rendering the initializer in assembly? it doesn't hurt to be more explicit, of course), so okay, it's good to do. But I assume that similar code is required in order to explicitly zero out any padding when dynamically initializing a local variable. Also, is adding constant padding fields retroactively really the best way to handle this instead of just expecting the constant emitter to fill them in itself? https://github.com/llvm/llvm-project/pull/97121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C] Disable use of NRVO (PR #101038)
rjmccall wrote: > Should we try to convince at least the GCC folks that they're getting this > wrong too? It does feel like a wider conversation is necessary, yeah. The fact that everybody is doing this even on x86_64 where it's pretty incontrovertibly forbidden seems significant. https://github.com/llvm/llvm-project/pull/101038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C] Disable use of NRVO (PR #101038)
rjmccall wrote: > > C++ does have some restrictions on accessing objects that are being > > initialized through other names. It's possible that they're strong enough > > to satisfy the ABI rule here through a sort of reverse of the normal > > analysis: basically, any program that would violate the ABI rule is > > actually incorrect because of the semantic restriction. If not, I think we > > may need to force copies of trivial return types in the caller in C++. We > > can consider that separately, though. > > I suspect it isn't legal to actually access the object through another > pointer while it's being constructed, but that doesn't help if the user is > just doing a pointer equality comparison. Ah, good point. > And C++17 rules forbid a copy on the caller side: "guaranteed copy elision" > means semantically, there is no copy. An extra copy is permitted for trivially-copyable types; otherwise we'd be required to pass and return all classes indirectly. https://github.com/llvm/llvm-project/pull/101038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C] Disable use of NRVO (PR #101038)
rjmccall wrote: I accept your pedantry. > Do we support ABIs that don't have a rule like x86_64's? The ARM and ARM64 AAPCSs specify that "The memory to be used for the result may be modified at any point during the function call" and "The callee may modify the result memory block at any point during the execution of the subroutine", respectively. I would interpret both to have a similar overall effect as the x86-64 psABI: - the function is not required to only assign to the memory immediately before returning - therefore doing earlier assignments must not have a semantic impact - therefore the caller must not pass memory that is legal to otherwise access during the call I checked a handful of other major ABIs (i386, MIPs, and PPC), and they don't say anything directly about it. Typically, the absence of a statement must be interpreted as a lack of a guarantee. In this case, however, I would argue it is more plausible to infer that the ABI authors did not consider the possibility of passing a pointer to aliased memory for the return value, and therefore we should interpret their silence as an implicit statement that compilers are not permitted to do that. There *are* plausible arguments for the *reverse* ABI rule from x86-64 here. For example, since trivial types don't distinguish initialization and assignment, an ABI rule that required indirect return values to be initialized as the final step in the callee would permit simple assignments in the caller (e.g. `x = foo();`) to be implemented by passing `` as the indirect return pointer. But a compiler couldn't even think about doing that without a strong, explicit guarantee in the ABI that functions can't interleave the initialization of the return value with arbitrary expression evaluation. (It's also a trade-off, because it forces the compiler to emit complex return values (like a compound literal) into temporaries that it will copy at the end of the function.) So there's really no reason for an ABI to intentionally try to split the difference here — it's just putting itself into the worst of all worlds. https://github.com/llvm/llvm-project/pull/101038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C] Disable use of NRVO (PR #101038)
rjmccall wrote: My argument above is that the x86-64 psABI requires us to not pass `` as the indirect return address. Following that rule fixes the problem here even if we keep doing NRVO. I think the approximate shape of the correct fix here is to: (1) Use the function Eli pointed out to check whether the initializer refers to the variable being initialized. (2) Propagate that information down in CodeGen when we're emitting into an address. (3) When emitting a call into an address, if the return value is trivially copyable and the initialized address is potentially aliased, emit into a temporary and then copy. https://github.com/llvm/llvm-project/pull/101038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C] Disable use of NRVO (PR #101038)
rjmccall wrote: Note that we're forced to override that ABI rule in C++ sometimes — I believe we're now required (since C++11?) to elide a copy/move when initializing a normal object with a return value, and even if we weren't, it'd clearly be unreasonable compiler behavior to do an extra non-trivial operation here. I don't think the psABI intends that rule to have that effect anyway. This does raise a language interoperation question, though, related to the tractability note I made about (2) above: - You can call a function to initialize a global variable in C++. - That function can be implemented in C, in which case it will expect the aliasing rule to still apply to its return value slot. - We cannot do any sort of local analysis on a global variable to prove that the variable is not accessed during its initializer. C++ does have some restrictions on accessing objects that are being initialized through other names. It's possible that they're strong enough to satisfy the ABI rule here through a sort of reverse of the normal analysis: basically, any program that would violate the ABI rule is actually incorrect because of the semantic restriction. If not, I think we may need to force copies of trivial return types in the caller in C++. We can consider that separately, though. https://github.com/llvm/llvm-project/pull/101038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C] Disable use of NRVO (PR #101038)
https://github.com/rjmccall requested changes to this pull request. We shouldn't merge this PR unless we need an immediate fix, which seems unlikely given how longstanding this behavior is. https://github.com/llvm/llvm-project/pull/101038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C] Disable use of NRVO (PR #101038)
rjmccall wrote:
I agree that the standard does not appear to permit this in C. There are three
ways to fix this:
1. Disable NRVO in the callee just in case the address being initialized is
aliased.
2. Disable RVO in the caller when aliasing is possible.
3. Convince the committee that they should change the standard to permit these
simple optimizations.
(1) also requires disabling copy elision in some cases. It is not possible to
directly observe the address of the compound literal in `return (struct S) {
E1, E2 };`, as the other example does, but E1 or E2 might assign to a field of
the alias, and such modifications must be overwritten when the return occurs.
It is tractable to implement (2) with a simple local escape analysis of the
variable's initializer. This would *not* be tractable if the initialized
variable were a global variable, but, fortunately, C does not permit global
variables to be initialized with non-constant expressions.
(3) would presumably take the form of either permitting objects to overlap in
certain situations, the same way that C++ permits copy elision, or just
generally weakening object overlap rules during initialization.
Richard, I'm sorry to contradict you, but Aaron is correct to guess that this
is ABI-affecting: ABIs should and sometimes do specify whether an indirect
return address is allowed to be aliased. For example, the x86_64 psABI is
quite clear:
> If the type has class MEMORY, then the caller provides space for the return
> value and passes the address of this storage in %rdi as if it were the first
> argument to the function. In effect, this address becomes a “hidden” first
> argument. This storage must not overlap any data visible to the callee
> through other names than this argument.
So on x86_64, we are clearly required to at least do (2). I don't think we
want different rules on different targets unless we're forced to.
If (2) is implemented correctly, I believe there's no way to observe NRVO, so
we don't also need to implement (1).
https://github.com/llvm/llvm-project/pull/101038
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Generate nuw GEPs for struct member accesses (PR #99538)
rjmccall wrote: `CGBuilder` is just aiming to provide an `Address` overload of LLVM's `CreateStructGEP`. It seems wrong for two overloads to have subtly different behavior. Is there a reason why LLVM's `CreateStructGEP` doesn't add `nuw` itself? https://github.com/llvm/llvm-project/pull/99538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [PAC] Implement authentication for C++ member function pointers (PR #99576)
@@ -1036,9 +1155,32 @@ llvm::Constant *ItaniumCXXABI::BuildMemberPointer(const CXXMethodDecl *MD, // least significant bit of adj then makes exactly the same // discrimination as the least significant bit of ptr does for // Itanium. - MemPtr[0] = llvm::ConstantInt::get(CGM.PtrDiffTy, VTableOffset); - MemPtr[1] = llvm::ConstantInt::get(CGM.PtrDiffTy, - 2 * ThisAdjustment.getQuantity() + 1); + + // We cannot use the Itanium ABI's representation for virtual member + // function pointers under pointer authentication because it would + // require us to store both the virtual offset and the constant + // discriminator in the pointer, which would be immediately vulnerable + // to attack. Instead we introduce a thunk that does the virtual dispatch + // and store it as if it were a non-virtual member function. This means + // that virtual function pointers may not compare equal anymore, but + // fortunately they aren't required to by the standard, and we do make + // a best-effort attempt to re-use the thunk. + // + // To support interoperation with code in which pointer authentication + // is disabled, derefencing a member function pointer must still handle + // the virtual case, but it can use a discriminator which should never + // be valid. + const auto = + CGM.getCodeGenOpts().PointerAuth.CXXMemberFunctionPointers; + if (Schema) +MemPtr[0] = llvm::ConstantExpr::getPtrToInt( +getSignedVirtualMemberFunctionPointer(MD), CGM.PtrDiffTy); + else +MemPtr[0] = llvm::ConstantInt::get(CGM.PtrDiffTy, VTableOffset); + // Don't set the LSB of adj to 1 if pointer authentication for member rjmccall wrote: The LSB of the offset indicates whether the stored function value is an offset within the v-table (normally used for virtual member functions) or a function pointer (normally used for non-virtual member functions). Because we are emitting the member function pointer with a function pointer to a thunk, we must clear the bit, or else calls will try to interpret the function pointer as an offset within the v-table. https://github.com/llvm/llvm-project/pull/99576 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Add metadata for load from reference (PR #98746)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/98746 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Add metadata for load from reference (PR #98746)
@@ -2799,9 +2799,37 @@ CodeGenFunction::EmitLoadOfReference(LValue RefLVal,
llvm::LoadInst *Load =
Builder.CreateLoad(RefLVal.getAddress(), RefLVal.isVolatile());
CGM.DecorateInstructionWithTBAA(Load, RefLVal.getTBAAInfo());
- return makeNaturalAddressForPointer(Load,
RefLVal.getType()->getPointeeType(),
- CharUnits(), /*ForPointeeType=*/true,
- PointeeBaseInfo, PointeeTBAAInfo);
+ QualType PTy = RefLVal.getType()->getPointeeType();
+ if (!PTy->isIncompleteType() && PTy->isConstantSizeType()) {
+llvm::LLVMContext = getLLVMContext();
+llvm::MDBuilder MDB(Ctx);
+// Emit !dereferenceable metadata
+Load->setMetadata(
+llvm::LLVMContext::MD_dereferenceable,
+llvm::MDNode::get(Ctx,
+ MDB.createConstant(llvm::ConstantInt::get(
+ Builder.getInt64Ty(),
rjmccall wrote:
Nikita is right. It is undefined behavior to bind a reference to an invalid
object, but it is not undefined behavior to allow that object to be destroyed
during the scope of a reference. This same limitation applies to reference
parameters, so we may have existing miscompiles here. (IIUC, it's not
surprising that such bugs could linger because `dereferenceable` only has any
effect on relatively specific code patterns.)
The only places we can use `dereferenceable` are when we have an ABI guarantee
about the lifetime of the pointee, such as when the ABI indirects an aggregate
argument or return value.
For references, we could use a weaker form of `dereferenceable` that accounts
for object invalidation. Conservatively, we would have to treat all calls and
synchronization as removing dereferenceability. That would still allow us to
optimize functions with no function calls (or only function calls known to not
invalidate any objects).
https://github.com/llvm/llvm-project/pull/98746
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] Don't drop weak_import from a declaration that follows a declaration directly contained in a linkage-specification (PR #85886)
https://github.com/rjmccall approved this pull request. https://github.com/llvm/llvm-project/pull/85886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] Don't drop weak_import from a declaration that follows a declaration directly contained in a linkage-specification (PR #85886)
https://github.com/rjmccall approved this pull request. LGTM. The (existing) diagnostic wording does seem a little awkward, so if you want to put effort into cleaning that up, it'd be welcome, but I won't hold up this fix. https://github.com/llvm/llvm-project/pull/85886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][NFC] Move more functions from `Sema` to `SemaObjC` (PR #97172)
rjmccall wrote: I agree that blocks don't belong in the ObjC segment. https://github.com/llvm/llvm-project/pull/97172 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Use different memory layout type for _BitInt(N) in LLVM IR (PR #91364)
https://github.com/rjmccall approved this pull request. https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Use different memory layout type for _BitInt(N) in LLVM IR (PR #91364)
rjmccall wrote: > > Okay, so x86_64 describes it in byte terms and says they're little-endian, > > which is consistent with the overall target. Interestingly, it does not > > guarantee the content of the excess bits. The code-generation in this patch > > is consistent with that: the extension we do is unnecessary but allowed, > > and then we truncate it away after load. If we ever add some way to tell > > the backend that a truncation is known to be reversing a > > sign/zero-extension, we'll need to not set it on this target. > > FYI this already exists in the form of `trunc nuw` / `trunc nsw`. (Though > it's not fully optimized yet.) Ah, neat. Mariya, would you mind looking into setting this properly on the truncates we're doing here? It'd be fine to do that as a follow-up; no need to hold up this PR for it. You'll need some kind of target hook to tell us whether to set it or not. Probably that ought to go in the Basic TargetInfo just so all of the target-specific ABI configuration is done in one place. https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C2y] Add documentation to conform to WG14 N3262; NFC (PR #98146)
https://github.com/rjmccall approved this pull request. https://github.com/llvm/llvm-project/pull/98146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
rjmccall wrote: Given all that, I feel pretty comfortable relying on using LLVM's `i96` stores and so on. I do worry some that we're eventually going to run into a target where the `_BitInt` ABI does not match what LLVM wants to generate for `i96` load/store, but we should be able to generalize this so that targets can override the `_BitInt` operations pretty easily. https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
rjmccall wrote: Okay, so x86_64 describes it in byte terms and says they're little-endian, which is consistent with the overall target. Interestingly, it does not guarantee the content of the excess bits. The code-generation in this patch is consistent with that: the extension we do is unnecessary but allowed, and then we truncate it away after load. If we ever add some way to tell the backend that a truncation is known to be reversing a sign/zero-extension, we'll need to not set it on this target. 32-bit and 64-bit ARM describe it in terms of smaller units, but the units are expressly laid out according to the overall endianness of the target, which composes to mean that the bytes overall are also laid out according to that endianness. https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C2y] Add documentation to conform to WG14 N3262; NFC (PR #98146)
rjmccall wrote: Ah right, I'd forgotten that some ABIs use that array trick to get it to pass by reference, and you're right that that makes it ill-formed to simply assign around. I like your idea of specifically making it UB to copy with `memcpy` etc and just advising that people use va_copy. https://github.com/llvm/llvm-project/pull/98146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -89,7 +89,7 @@ void CodeGenTypes::addRecordTypeName(const RecordDecl *RD, /// ConvertType in that it is used to convert to the memory representation for /// a type. For example, the scalar representation for _Bool is i1, but the /// memory representation is usually i8 or i32, depending on the target. rjmccall wrote: ```suggestion /// memory representation is usually i8 or i32, depending on the target. /// /// We generally assume that the alloc size of this type under the LLVM /// data layout is the same as the size of the AST type. The alignment /// does not have to match: Clang should always use explicit alignments /// and packed structs as necessary to produce the layout it needs. /// But the size does need to be exactly right or else things like struct /// layout will break. ``` https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -107,17 +107,52 @@ llvm::Type *CodeGenTypes::ConvertTypeForMem(QualType T,
bool ForBitField) {
return llvm::IntegerType::get(FixedVT->getContext(), BytePadded);
}
- // If this is a bool type, or a bit-precise integer type in a bitfield
- // representation, map this integer to the target-specified size.
rjmccall wrote:
Let's keep this comment; we just need to update it a little:
```
// If T is _Bool or a _BitInt type, ConvertType will produce an IR type
// with the exact semantic bit-width of the AST type; for example,
// _BitInt(17) will turn into i17. In memory, however, we need to store
// such values extended to their full storage size as decided by AST
// layout; this is an ABI requirement. Ideally, we would always use an
// integer type that's just the bit-size of the AST type; for example, if
// sizeof(_BitInt(17)) == 4, _BitInt(17) would turn into i32. That is what's
// returned by convertTypeForLoadStore. However, that type does not
// always satisfy the size requirement on memory representation types
// describe above. For example, a 32-bit platform might reasonably set
// sizeof(_BitInt(65)) == 12, but i96 is likely to have to have an alloc size
// of 16 bytes in the LLVM data layout. In these cases, we simply return
// a byte array of the appropriate size.
```
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
https://github.com/rjmccall commented: This is generally looking great, and I think it's ready to go as soon as you can finish the tests. (You said you weren't able to update all the tests — did you have questions about the remaining tests?) I did have a thought, though. Are we confident that the in-memory layout that LLVM is using for these large integer types matches the layout specified by the ABI? I know this patch makes the overall sizes match, but there's also an "endianness" question. When LLVM stores an i96 on a 32-bit machine, I assume it does three 32-bit stores, and those stores probably match the overall endianness of the target; for example, on a little-endian architecture, it stores the low 32 bits at offset 0, the middle 32 bits at offset 4, and the high 32 bits at offset 8. I just want to make sure that the ABI specification for _BitInt actually matches that. https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -1886,6 +1896,29 @@ llvm::Constant
*ConstantEmitter::emitForMemory(CodeGenModule ,
return Res;
}
+ if (destType->isBitIntType()) {
+if (CGM.getTypes().typeRequiresSplitIntoByteArray(destType, C->getType()))
{
+ // Long _BitInt has array of bytes as in-memory type.
+ // So, split constant into individual bytes.
+ ConstantAggregateBuilder Builder(CGM);
+ llvm::Type *DesiredTy = CGM.getTypes().ConvertTypeForMem(destType);
+ llvm::Type *LoadStoreTy =
rjmccall wrote:
Right. Momchil, your description is actually a totally reasonable way of
understanding what `convertTypeForLoadStore` *means* for `_BitInt`: the
load/store type is an integer type that has the same width as the in-memory
type. We're just recognizing that as a more general concept.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C2y] Add documentation to conform to WG14 N3262; NFC (PR #98146)
rjmccall wrote: Oh, I completely spaced on this before, but of course there *are* constraints on `va_list` in the standard: `va_list`s are passed by value to functions like `vprintf`. That, of course, requires the value to be primitively copied. If you call `vprintf(format, args)`, the standard says the value of `args` is indeterminate after the call. It also specifically says that the `v*printf` functions do not call `va_end`. As a result, the standard is clearly requiring that `va_list` be correctly "borrowed" when you pass it as an argument. It doesn't directly say that any other forms of relocation are allowed, but I think it would be surprising if argument passing worked but builtin assignment / initialization didn't, and I'm willing to say that we would never implement such a thing in Clang. `memcpy` is a separate question: you can imagine an ABI that requires these primitive copies to have special behavior, e.g. by storing the pointers with address-sensitive `__ptrauth` to resist data corruption. The ownership of the value must be understood in C terms, not in terms of a C++-like object model. A `va_list` value potentially has ownership of some resource that must be destroyed by `va_end`. You don't have to destroy the "original" `va_list`, but you do have to destroy some primitive copy of it. Whether changes to a `va_list` made by `va_arg` are observed in primitive copies is indeterminate. `va_copy` produces an independent `va_list` that does not observe subsequent changes to the original `va_list` value(s) and must be separately destroyed. How precisely we want to document this is an interesting question, but I do think it has to be more subtle than "primitive copies are UB", because we clearly can't make `vprintf` invalid. https://github.com/llvm/llvm-project/pull/98146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C2y] Add documentation to conform to WG14 N3262; NFC (PR #98146)
rjmccall wrote: > > _Every_ `va_list` stores pointers; otherwise, it wouldn't be able to > > support an arbitrary number of arguments. Copying just copies the pointers, > > and that's fine because the memory they point to is immutable and always > > outlives the `va_list`. You can imagine a `va_list` implementation that > > doesn't have these properties, but it's hard. > > But do separate va_list objects within the same function always have the same > pointer _values_? I was thinking the "saved state" pointers might actually > point to different memory regions to support different restoration points, > but I could be way off base there. The pointer values in a specific `va_list` can and often do change when you call `va_arg`, but I am not aware of any targets where they do not always point to the same regions. Every `va_list` ABI I know has a pointer to the stack argument area, and those pointers *must* point there because the stack argument area cannot be either copied or relocated. Some ABIs also require certain argument registers to be spilled to the stack in the prologue, and then a pointer to that spill area (or multiple areas, depending on target) is also written into the `va_list`. In theory, a compiler could produce multiple copies of that spill area, or an ABI could require it to be allocate on the heap or copied by `va_copy`. However, there's no good reason to do either of those things: the `va_list` can never validly escape the variadic function because of the limitation on the stack argument area, and so the spill area might as well also go in a unique location on the stack. > > > I'm not super happy about "this is well-defined if you happen to be on a > > > target that makes it well-defined" because there's basically no > > > reasonable way for a user to find that information out themselves. > > > > > > Well, if we're going to document that this is allowed, we should document > > what targets it's allowed on. I'm not arguing that we shouldn't document it. > > I had the unpleasant experience of trying to figure out what targets it's > allowed on. We have five links to ABI resources regarding va_list in > > https://github.com/llvm/llvm-project/blob/746f5726158d31aeeb1fd12b226dc869834a7ad2/clang/include/clang/Basic/TargetInfo.h#L319 > > ; four of the links are dead and one of the targets is unsupported as of 2022 > (thus has no documentation whatsoever) but its va_list type continues to be > used outside of that target ( > https://github.com/llvm/llvm-project/blob/a1d73ace13a20ed122a66d3d59f0cbae58d0f669/clang/lib/Basic/Targets/Le64.h#L41 > > ). > Given the ease of calling `va_copy`, the fact that C had implicit UB here for > ~40 years, and it's going to be a slog to try to nail down every single > target's behavior, I still think documenting it as explicit UB is reasonable. > If users or target owners want a different behavior, then they could give a > use case for why and we could re-assess at that point. All we're effectively > saying with that documentation is "if you do it, we don't promise it will > work" and we already effectively say that with our existing documentation > when we say `It is undefined behavior to call this function with a list that > has not been initialized by either __builtin_va_start or __builtin_va_copy.` > > Alternatively, we could document that it's supported on all targets, test > whatever codegen Clang emits, assume all the targets support this, and then > document any unsupported targets discovered in the wild as explicitly > unsupported. I'm a bit less comfortable with that, but when both the codegen > code owners tell me they it's very unlikely this _won't_ work on any targets > we care about, I can certainly trust that expertise! I have no objection to requiring `va_copy` and documenting primitive copies as undefined behavior. It is the easiest thing to do, and we can clearly revisit it later if we want. https://github.com/llvm/llvm-project/pull/98146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C2y] Add documentation to conform to WG14 N3262; NFC (PR #98146)
rjmccall wrote: *Every* `va_list` stores pointers; otherwise, it wouldn't be able to support an arbitrary number of arguments. Copying just copies the pointers, and that's fine because the memory they point to is immutable and always outlives the `va_list`. You can imagine a `va_list` implementation that doesn't have these properties, but it's hard. > I'm not super happy about "this is well-defined if you happen to be on a > target that makes it well-defined" because there's basically no reasonable > way for a user to find that information out themselves. Well, if we're going to document that this is allowed, we should document what targets it's allowed on. I'm not arguing that we shouldn't document it. https://github.com/llvm/llvm-project/pull/98146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C2y] Add documentation to conform to WG14 N3262; NFC (PR #98146)
rjmccall wrote: I also have trouble imagining why a target would ever want to make `va_copy` a non-trivial operation, and I suspect that in practice programmers do not reliably call `va_end` to clean up their iterations. In general, I would say that platforms should be moving towards making varargs *simpler* by just treating variadic arguments differently from required arguments in the ABI, the way AArch64 originally did and still does on Apple platforms. I wouldn't want to lock us out of supporting a target that required a non-trivial `va_copy` for whatever awful reason, but we could be clear that it's only UB on such targets. I don't think we actually gain anything from making it UB; I can't imagine what optimizations that would enable. https://github.com/llvm/llvm-project/pull/98146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -2093,17 +2107,10 @@ void CodeGenFunction::EmitStoreOfScalar(llvm::Value
*Value, Address Addr,
llvm::Type *SrcTy = Value->getType();
if (const auto *ClangVecTy = Ty->getAs()) {
auto *VecTy = dyn_cast(SrcTy);
-if (VecTy && ClangVecTy->isExtVectorBoolType()) {
- auto *MemIntTy = cast(Addr.getElementType());
- // Expand to the memory bit width.
- unsigned MemNumElems = MemIntTy->getPrimitiveSizeInBits();
- // --> .
- Value = emitBoolVecConversion(Value, MemNumElems, "insertvec");
- // --> iP.
- Value = Builder.CreateBitCast(Value, MemIntTy);
-} else if (!CGM.getCodeGenOpts().PreserveVec3Type) {
+if (!CGM.getCodeGenOpts().PreserveVec3Type) {
// Handle vec3 special.
- if (VecTy && cast(VecTy)->getNumElements() == 3) {
+ if (!ClangVecTy->isExtVectorBoolType() && VecTy &&
rjmccall wrote:
`VecTy` is the cheapest thing to check here, you should continue to check it
first.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -1774,6 +1774,18 @@ llvm::Constant
*ConstantEmitter::emitForMemory(CodeGenModule ,
return Res;
}
+ if (const auto *BIT = destType->getAs()) {
+if (BIT->getNumBits() > 128) {
+ // Long _BitInt has array of bytes as in-memory type.
+ ConstantAggregateBuilder Builder(CGM);
+ llvm::Type *DesiredTy = CGM.getTypes().ConvertTypeForMem(destType);
+ auto *CI = cast(C);
rjmccall wrote:
The test case here is just going to be something like `_SomeSplitBitIntType x =
(unsigned long) `. What code do we actually produce for this?
Sometimes we'll be able to fall back on dynamic initialization, but that's not
always an option.
Ideally, it's just invalid to do something like that. It certainly needs to be
diagnosed if the integer type is narrower than the pointer, and wider is also
problematic, although less so and in a different way.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -1,12 +1,25 @@
-// RUN: %clang_cc1 -triple x86_64-gnu-linux -O3 -disable-llvm-passes
-emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK64
-// RUN: %clang_cc1 -triple x86_64-windows-pc -O3 -disable-llvm-passes
-emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK64
-// RUN: %clang_cc1 -triple i386-gnu-linux -O3 -disable-llvm-passes -emit-llvm
-o - %s | FileCheck %s --check-prefixes=CHECK,LIN32
-// RUN: %clang_cc1 -triple i386-windows-pc -O3 -disable-llvm-passes -emit-llvm
-o - %s | FileCheck %s --check-prefixes=CHECK,WIN32
+// RUN: %clang_cc1 -std=c23 -triple x86_64-gnu-linux -O3 -disable-llvm-passes
-emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK64,LIN64
+// RUN: %clang_cc1 -std=c23 -triple x86_64-windows-pc -O3 -disable-llvm-passes
-emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK64,WIN64
+// RUN: %clang_cc1 -std=c23 -triple i386-gnu-linux -O3 -disable-llvm-passes
-emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,LIN32
+// RUN: %clang_cc1 -std=c23 -triple i386-windows-pc -O3 -disable-llvm-passes
-emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,WIN32
+
+// CHECK64: %struct.S1 = type { i17, [4 x i8], [24 x i8] }
+// WIN32: %struct.S1 = type { i17, [4 x i8], [24 x i8] }
+// LIN32: %struct.S1 = type { i17, [20 x i8] }
+// CHECK64: %struct.S2 = type { [40 x i8], i32, [4 x i8] }
+// WIN32: %struct.S2 = type { [40 x i8], i32, [4 x i8] }
+// LIN32: %struct.S2 = type { [36 x i8], i32 }
+// LIN64: %struct.S3 = type { [17 x i8], [7 x i8] }
+// WIN64: %struct.S3 = type { [24 x i8] }
//GH62207
unsigned _BitInt(1) GlobSize1 = 0;
// CHECK: @GlobSize1 = {{.*}}global i1 false
+// CHECK64: @__const.foo.A = private unnamed_addr constant { i17, [4 x i8], <{
i8, [23 x i8] }> } { i17 1, [4 x i8] undef, <{ i8, [23 x i8] }> <{ i8 -86, [23
x i8] zeroinitializer }> }, align 8
rjmccall wrote:
Hmm. Why is `i17` in the member layout here? Doesn't the ABI require these
small `_BitInt`s to be extended out to the nearest power of 2? So the
load/store type needs to be `i32`, and that should work as the memory layout
type as well.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -609,9 +609,25 @@ bool ConstStructBuilder::AppendBytes(CharUnits
FieldOffsetInChars,
return Builder.add(InitCst, StartOffset + FieldOffsetInChars,
AllowOverwrite);
}
-bool ConstStructBuilder::AppendBitField(
-const FieldDecl *Field, uint64_t FieldOffset, llvm::ConstantInt *CI,
-bool AllowOverwrite) {
+bool ConstStructBuilder::AppendBitField(const FieldDecl *Field,
+uint64_t FieldOffset, llvm::Constant
*C,
+bool AllowOverwrite) {
+
+ llvm::ConstantInt *CI = dyn_cast(C);
+ if (!CI) {
+// Constants for long _BitInt types are split into individual bytes.
rjmccall wrote:
```suggestion
// Constants for _BitInt types are sometimes split into individual bytes.
```
Let's try to remember that this isn't *necessarily* a function of the size of
the `_BitInt` type.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Implement function pointer type discrimination (PR #96992)
@@ -3140,6 +3140,269 @@
ASTContext::getPointerAuthVTablePointerDiscriminator(const CXXRecordDecl *RD) {
return llvm::getPointerAuthStableSipHash(Str);
}
+/// Encode a function type for use in the discriminator of a function pointer
+/// type. We can't use the itanium scheme for this since C has quite permissive
+/// rules for type compatibility that we need to be compatible with.
+///
+/// Formally, this function associates every function pointer type T with an
+/// encoded string E(T). Let the equivalence relation T1 ~ T2 be defined as
+/// E(T1) == E(T2). E(T) is part of the ABI of values of type T. C type
+/// compatibility requires equivalent treatment under the ABI, so
+/// CCompatible(T1, T2) must imply E(T1) == E(T2), that is, CCompatible must be
+/// a subset of ~. Crucially, however, it must be a proper subset because
+/// CCompatible is not an equivalence relation: for example, int[] is
compatible
+/// with both int[1] and int[2], but the latter are not compatible with each
+/// other. Therefore this encoding function must be careful to only distinguish
+/// types if there is no third type with which they are both required to be
+/// compatible.
+static void encodeTypeForFunctionPointerAuth(ASTContext , raw_ostream ,
+ QualType QT) {
+ // FIXME: Consider address space qualifiers.
+ const Type *T = QT.getCanonicalType().getTypePtr();
+
+ // FIXME: Consider using the C++ type mangling when we encounter a construct
+ // that is incompatible with C.
+
+ switch (T->getTypeClass()) {
+ case Type::Atomic:
+return encodeTypeForFunctionPointerAuth(
+Ctx, OS, cast(T)->getValueType());
+
+ case Type::LValueReference:
+OS << "R";
+encodeTypeForFunctionPointerAuth(Ctx, OS,
+ cast(T)->getPointeeType());
+return;
+ case Type::RValueReference:
+OS << "O";
+encodeTypeForFunctionPointerAuth(Ctx, OS,
+ cast(T)->getPointeeType());
+return;
+
+ case Type::Pointer:
+// C11 6.7.6.1p2:
+// For two pointer types to be compatible, both shall be identically
+// qualified and both shall be pointers to compatible types.
+// FIXME: we should also consider pointee types.
+OS << "P";
+return;
+
+ case Type::ObjCObjectPointer:
+ case Type::BlockPointer:
+OS << "P";
+return;
+
+ case Type::Complex:
+OS << "C";
+return encodeTypeForFunctionPointerAuth(
+Ctx, OS, cast(T)->getElementType());
+
+ case Type::VariableArray:
+ case Type::ConstantArray:
+ case Type::IncompleteArray:
+ case Type::ArrayParameter:
+// C11 6.7.6.2p6:
+// For two array types to be compatible, both shall have compatible
+// element types, and if both size specifiers are present, and are
integer
+// constant expressions, then both size specifiers shall have the same
+// constant value [...]
+//
+// So since ElemType[N] has to be compatible ElemType[], we can't encode
the
+// width of the array.
+OS << "A";
+return encodeTypeForFunctionPointerAuth(
+Ctx, OS, cast(T)->getElementType());
+
+ case Type::ObjCInterface:
+ case Type::ObjCObject:
+OS << "";
+return;
+
+ case Type::Enum:
+// C11 6.7.2.2p4:
+// Each enumerated type shall be compatible with char, a signed integer
+// type, or an unsigned integer type.
+//
+// So we have to treat enum types as integers.
+OS << "i";
+return;
+
+ case Type::FunctionNoProto:
+ case Type::FunctionProto: {
+// C11 6.7.6.3p15:
+// For two function types to be compatible, both shall specify compatible
+// return types. Moreover, the parameter type lists, if both are present,
+// shall agree in the number of parameters and in the use of the ellipsis
+// terminator; corresponding parameters shall have compatible types.
+//
+// That paragraph goes on to describe how unprototyped functions are to be
+// handled, which we ignore here. Unprototyped function pointers are hashed
+// as though they were prototyped nullary functions since thats probably
+// what the user meant. This behavior is non-conforming.
+// FIXME: If we add a "custom discriminator" function type attribute we
+// should encode functions as their discriminators.
+OS << "F";
+auto *FuncType = cast(T);
+encodeTypeForFunctionPointerAuth(Ctx, OS, FuncType->getReturnType());
+if (auto *FPT = dyn_cast(FuncType)) {
+ for (QualType Param : FPT->param_types()) {
+Param = Ctx.getSignatureParameterType(Param);
+encodeTypeForFunctionPointerAuth(Ctx, OS, Param);
+ }
+ if (FPT->isVariadic())
+OS << "z";
+}
+OS << "E";
+return;
+ }
+
+ case Type::MemberPointer: {
+OS << "M";
+auto *MPT = T->getAs();
+encodeTypeForFunctionPointerAuth(Ctx, OS, QualType(MPT->getClass(), 0));
+
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -1774,6 +1784,22 @@ llvm::Constant
*ConstantEmitter::emitForMemory(CodeGenModule ,
return Res;
}
+ if (destType->isBitIntType()) {
+if (!CGM.getTypes().LLVMTypeLayoutMatchesAST(destType, C->getType())) {
+ // Long _BitInt has array of bytes as in-memory type.
+ // So, split constant into individual bytes.
+ ConstantAggregateBuilder Builder(CGM);
+ llvm::Type *DesiredTy = CGM.getTypes().ConvertTypeForMem(destType);
+ // LLVM type doesn't match AST type only for big enough _BitInts, these
+ // types don't appear in constant expressions involving ptrtoint, so it
+ // is safe to expect a constant int here.
+ auto *CI = cast(C);
+ llvm::APInt Value = CI->getValue();
+ Builder.addBits(Value, /*OffsetInBits=*/0, /*AllowOverwrite=*/false);
+ return Builder.build(DesiredTy, /*AllowOversized*/ false);
rjmccall wrote:
`C` is of the scalar type, not the load/store type, right? So tail-padding
with `zeroinitializer` is still wrong:
- Given a negative value, we need to fill the "padding" with 1s, not 0s.
- On a big-endian target, the padding needs to come before the value, not after
it.
Both of these are fixed by sign/zero-extending the constant to the load/store
type.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
https://github.com/rjmccall requested changes to this pull request. Getting very close. https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -118,6 +124,39 @@ llvm::Type *CodeGenTypes::ConvertTypeForMem(QualType T,
bool ForBitField) {
return R;
}
+bool CodeGenTypes::typeRequiresSplitIntoByteArray(QualType ASTTy,
+ llvm::Type *LLVMTy) {
+ if (!LLVMTy)
+LLVMTy = ConvertType(ASTTy);
+
+ CharUnits ASTSize = Context.getTypeSizeInChars(ASTTy);
+ CharUnits LLVMSize =
+ CharUnits::fromQuantity(getDataLayout().getTypeAllocSize(LLVMTy));
+ return ASTSize != LLVMSize;
+}
+
+llvm::Type *CodeGenTypes::convertTypeForLoadStore(QualType T,
+ llvm::Type *LLVMTy) {
+ if (!LLVMTy)
+LLVMTy = ConvertType(T);
+
+ if (!T->isBitIntType() && LLVMTy->isIntegerTy(1))
+return llvm::IntegerType::get(getLLVMContext(),
+ (unsigned)Context.getTypeSize(T));
+
+ if (T->isBitIntType()) {
rjmccall wrote:
You can avoid a second call to `isBitIntType()` by just putting this block
before the one above it.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/91364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -610,8 +610,26 @@ bool ConstStructBuilder::AppendBytes(CharUnits
FieldOffsetInChars,
}
bool ConstStructBuilder::AppendBitField(
-const FieldDecl *Field, uint64_t FieldOffset, llvm::ConstantInt *CI,
+const FieldDecl *Field, uint64_t FieldOffset, llvm::Constant *C,
bool AllowOverwrite) {
+
+ llvm::ConstantInt *CI = nullptr;
rjmccall wrote:
```suggestion
llvm::ConstantInt *CI = dyn_cast(C);
```
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -128,6 +128,16 @@ class CodeGenTypes {
/// memory representation is usually i8 or i32, depending on the target.
llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false);
+ /// Check that size and ABI alignment of given LLVM type matches size and
+ /// alignment of given AST type. If they don't, values of the type need to be
+ /// emitted as byte array.
+ bool typeRequiresSplitIntoByteArray(QualType ASTTy,
+ llvm::Type *LLVMTy = nullptr);
+
+ /// For AST types with special memory representation returns type
+ /// that ought to be used for load and store operations.
rjmccall wrote:
```
/// Given that T is a scalar type, return the IR type that should
/// be used for load and store operations. For example, this might
/// be i8 for _Bool or i96 for _BitInt(65). The store size of the
/// load/store type (as reported by LLVM's data layout) is always
/// the same as the alloc size of the memory representation type
/// returned by ConvertTypeForMem.
///
/// As an optimization, if you already know the scalar value type
/// for T (as would be returned by ConvertType), you can pass
/// it as the second argument so that it does not need to be
/// recomputed in common cases where the value type and
/// load/store type are the same.
```
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -128,6 +128,16 @@ class CodeGenTypes {
/// memory representation is usually i8 or i32, depending on the target.
llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false);
+ /// Check that size and ABI alignment of given LLVM type matches size and
+ /// alignment of given AST type. If they don't, values of the type need to be
+ /// emitted as byte array.
rjmccall wrote:
```suggestion
/// Check whether the given type needs to be laid out in memory
/// using an opaque byte-array type because its load/store type
/// does not have the correct alloc size in the LLVM data layout.
/// If this is false, the load/store type (convertTypeForLoadStore)
/// and memory representation type (ConvertTypeForMem) will
/// be the same type.
```
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CodeGen] Add a flag to disable emitting block signature strings (PR #96944)
https://github.com/rjmccall approved this pull request. https://github.com/llvm/llvm-project/pull/96944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -1070,13 +1084,20 @@ CodeGenFunction::GenerateCXXGlobalInitFunc(llvm::Function *Fn, // Mark as initialized before initializing anything else. If the // initializers use previously-initialized thread_local vars, that's // probably supposed to be OK, but the standard doesn't say. - Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(),1), Guard); - - // The guard variable can't ever change again. + // Get the thread-local address via intrinsic. + if (IsTLS) +GuardAddr = GuardAddr.withPointer( +Builder.CreateThreadLocalAddress(Guard.getPointer()), +NotKnownNonNull); + Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(), 1), + GuardAddr); + + // Emit invariant start for TLS guard address. EmitInvariantStart( Guard.getPointer(), CharUnits::fromQuantity( - CGM.getDataLayout().getTypeAllocSize(GuardVal->getType(; + CGM.getDataLayout().getTypeAllocSize(GuardVal->getType())), + IsTLS); rjmccall wrote: I feel like we just need to file a DR with WG21 on this general question. The alternatives I can see: 1. Forbid TLVs in coroutines. Source-breaking. 2. Continue to eagerly initialize local TLVs in coroutines, but bind the name permanently to the TLV that was initialized. This means it is potentially a cross-thread reference after a suspension. It is the user's responsibility to make that not a problem. A semantics change for most (if not at all) existing compilers. 3. Like #2, but give references to the TLV undefined behavior if the thread has changed. The most compatible option, sadly. 4. Like #2, but make the program statically ill-formed if a suspension potentially intervenes between the initialization and a reference to the TLV. Source-breaking. Also a significant new implementation burden for compilers. 5. Switch local TLVs in coroutines to the same lazy-initialization model as global TLVs. Every reference to the variable dynamically resolves to the TLV for the current thread. Because of the model change, the variable is always initialized. A semantics change for most (if not all) existing compilers. 6. Like #5, but require the initializer to be a constant expression just to try to minimize the semantic impact. Note that a cross-thread reference can race not just with read/write or write/write conflicts between threads, but also with the destruction of the other thread. Because of the latter, it will be a potentially-dangling reference for most async-like use cases. https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -1070,13 +1084,20 @@ CodeGenFunction::GenerateCXXGlobalInitFunc(llvm::Function *Fn, // Mark as initialized before initializing anything else. If the // initializers use previously-initialized thread_local vars, that's // probably supposed to be OK, but the standard doesn't say. - Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(),1), Guard); - - // The guard variable can't ever change again. + // Get the thread-local address via intrinsic. + if (IsTLS) +GuardAddr = GuardAddr.withPointer( +Builder.CreateThreadLocalAddress(Guard.getPointer()), +NotKnownNonNull); + Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(), 1), + GuardAddr); + + // Emit invariant start for TLS guard address. EmitInvariantStart( Guard.getPointer(), CharUnits::fromQuantity( - CGM.getDataLayout().getTypeAllocSize(GuardVal->getType(; + CGM.getDataLayout().getTypeAllocSize(GuardVal->getType())), + IsTLS); rjmccall wrote: That's exactly backwards — it is semantically incorrect to access different guard variables at different points during the initialization; that does not correctly implement the guard pattern. Fortunately, I've been able to verify that it doesn't matter, because [expr.await]p3 says: > An *await-expression* shall not appear in the initializer of a block variable > with static or thread storage duration. Therefore, it is fine to just do a single use of `threadlocal.address`, and it's preferable because it's less code and likely easier to optimize. (Unfortunately, this restriction doesn't completely eliminate the coherence problems with coroutines and `thread_local`. In particular, a coroutine that declares a `thread_local` local variable can observe it in an uninitialized state if the thread has changed since the initialization, and I don't see anything in the standard that addresses this.) https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Implement function pointer type discrimination (PR #96992)
@@ -2220,6 +2220,11 @@ llvm::Constant
*ConstantLValueEmitter::emitPointerAuthPointer(const Expr *E) {
// The assertions here are all checked by Sema.
assert(Result.Val.isLValue());
+ auto *Base = Result.Val.getLValueBase().get();
+ if (auto *Decl = dyn_cast_or_null(Base)) {
+assert(Result.Val.getLValueOffset().isZero());
+return CGM.getRawFunctionPointer(Decl);
rjmccall wrote:
You mean to `ConstantEmitter`? I'd be okay with a flag that specifically
requests a raw pointer; that doesn't seem too error-prone.
https://github.com/llvm/llvm-project/pull/96992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -1070,13 +1084,20 @@ CodeGenFunction::GenerateCXXGlobalInitFunc(llvm::Function *Fn, // Mark as initialized before initializing anything else. If the // initializers use previously-initialized thread_local vars, that's // probably supposed to be OK, but the standard doesn't say. - Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(),1), Guard); - - // The guard variable can't ever change again. + // Get the thread-local address via intrinsic. + if (IsTLS) +GuardAddr = GuardAddr.withPointer( +Builder.CreateThreadLocalAddress(Guard.getPointer()), +NotKnownNonNull); + Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(), 1), + GuardAddr); + + // Emit invariant start for TLS guard address. EmitInvariantStart( Guard.getPointer(), CharUnits::fromQuantity( - CGM.getDataLayout().getTypeAllocSize(GuardVal->getType(; + CGM.getDataLayout().getTypeAllocSize(GuardVal->getType())), + IsTLS); rjmccall wrote: Yes, and in fact, sometimes that's required. The way I think about it is that `threadlocal.address` is a real dynamic operation that resolves the address of the thread-local variable for a specific thread: namely, the current thread at the execution point where `threadlocal.address` appears. When we're doing a complicated operation on a specific thread-local variable, it's important to call `threadlocal.address` at the right point and then use that result throughout the operation. Otherwise, a suspension in the middle of the operation will leave us working on a different thread-local variable at different points in the operation, which is not semantically allowed. In this case, we initialize a thread-local variable and then enter an invariant region for it. We need this to: 1. resolve the address of the TLV for the current thread, prior to performing any initialization; 2. initialize the TLV, which may include suspensions that change the current thread; and 3. enter an invariant region for the TLV we initialized, *not* the TLV for the new thread. The new thread's TLV may not yet be in an invariant region. Now, this may actually be moot, because I'm not sure it's actually allowed (or at least I'm not sure it *should* be allowed) to have a coroutine suspension in the middle of the initialization of a thread-local variable. The interaction of thread-locals with coroutines that actually switch threads is deeply problematic; notably, the TLV for the new thread can actually be uninitialized when you go to use it. I haven't checked what the standard says here, but honestly this might have to have undefined behavior, in which case we just need to make sure we generate reasonable code as long as the thread *doesn't* change. https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C2y] Claim conformance to WG14 N3254 (PR #97581)
rjmccall wrote: Yes, I agree that we can reasonably claim to support this for the indefinite future. This is something we would've seen ourselves as practically constrained from exploiting anyway, effectively making it well-defined even if the standard didn't act. https://github.com/llvm/llvm-project/pull/97581 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CodeGen] Add a flag to disable emitting block signature strings (PR #96944)
https://github.com/rjmccall commented: This all seems reasonable to me. Please add real documentation for this, though, not just a release note. https://github.com/llvm/llvm-project/pull/96944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -2933,7 +2933,8 @@ void ItaniumCXXABI::EmitThreadLocalInitFuncs( Guard->setAlignment(GuardAlign.getAsAlign()); CodeGenFunction(CGM).GenerateCXXGlobalInitFunc( -InitFunc, OrderedInits, ConstantAddress(Guard, CGM.Int8Ty, GuardAlign)); +InitFunc, OrderedInits, ConstantAddress(Guard, CGM.Int8Ty, GuardAlign), +Guard->isThreadLocal()); rjmccall wrote: ```suggestion /*IsTLS*/ true); ``` https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -1070,13 +1084,20 @@ CodeGenFunction::GenerateCXXGlobalInitFunc(llvm::Function *Fn, // Mark as initialized before initializing anything else. If the // initializers use previously-initialized thread_local vars, that's // probably supposed to be OK, but the standard doesn't say. - Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(),1), Guard); - - // The guard variable can't ever change again. + // Get the thread-local address via intrinsic. + if (IsTLS) +GuardAddr = GuardAddr.withPointer( +Builder.CreateThreadLocalAddress(Guard.getPointer()), +NotKnownNonNull); + Builder.CreateStore(llvm::ConstantInt::get(GuardVal->getType(), 1), + GuardAddr); + + // Emit invariant start for TLS guard address. EmitInvariantStart( Guard.getPointer(), CharUnits::fromQuantity( - CGM.getDataLayout().getTypeAllocSize(GuardVal->getType(; + CGM.getDataLayout().getTypeAllocSize(GuardVal->getType())), + IsTLS); rjmccall wrote: Just use `GuardAddr` here, please, instead of passing a flag down. https://github.com/llvm/llvm-project/pull/96633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -769,9 +777,10 @@ void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary)
{
CharUnits GuardAlign = CharUnits::One();
Guard->setAlignment(GuardAlign.getAsAlign());
GuardAddr = ConstantAddress(Guard, Int8Ty, GuardAlign);
+ IsTLS = Guard->isThreadLocal();
rjmccall wrote:
This is always just `false`. And I believe that's correct, module
initialization is global, not thread-local.
https://github.com/llvm/llvm-project/pull/96633
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -769,9 +777,10 @@ void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary)
{
CharUnits GuardAlign = CharUnits::One();
Guard->setAlignment(GuardAlign.getAsAlign());
GuardAddr = ConstantAddress(Guard, Int8Ty, GuardAlign);
+ IsTLS = Guard->isThreadLocal();
}
-CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits,
- GuardAddr);
+CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits,
GuardAddr,
+ IsTLS);
rjmccall wrote:
```suggestion
/*IsTLS*/ false);
```
https://github.com/llvm/llvm-project/pull/96633
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -1059,9 +1059,15 @@
CodeGenFunction::GenerateCXXGlobalInitFunc(llvm::Function *Fn,
if (Guard.isValid()) {
// If we have a guard variable, check whether we've already performed
// these initializations. This happens for TLS initialization functions.
- llvm::Value *GuardVal = Builder.CreateLoad(Guard);
- llvm::Value *Uninit = Builder.CreateIsNull(GuardVal,
- "guard.uninitialized");
+ Address GuardAddr = Guard;
+ if (auto *GV = dyn_cast(Guard.getPointer()))
rjmccall wrote:
Then you should pass down whether this is for TLS.
What I'm trying to avoid is some situation where we silently don't emit the
intrinsic for a thread local because the guard address we pass in is not
directly a `GlobalValue`.
https://github.com/llvm/llvm-project/pull/96633
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Emit distinct TBAA tags for pointers with different depths,types. (PR #76612)
https://github.com/rjmccall commented: Alright. LGTM, but let's ping @AaronBallman and @efriedma-quic. https://github.com/llvm/llvm-project/pull/76612 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Access tls_guard via llvm.threadlocal.address (PR #96633)
@@ -1059,9 +1059,15 @@
CodeGenFunction::GenerateCXXGlobalInitFunc(llvm::Function *Fn,
if (Guard.isValid()) {
// If we have a guard variable, check whether we've already performed
// these initializations. This happens for TLS initialization functions.
- llvm::Value *GuardVal = Builder.CreateLoad(Guard);
- llvm::Value *Uninit = Builder.CreateIsNull(GuardVal,
- "guard.uninitialized");
+ Address GuardAddr = Guard;
+ if (auto *GV = dyn_cast(Guard.getPointer()))
rjmccall wrote:
We need to do this unconditionally, and we should be able to do because `Guard`
is a constant address. But if there's some code pattern where that's not true,
we need to fix that code pattern rather than silently just not using this
intrinsic.
https://github.com/llvm/llvm-project/pull/96633
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Emit distinct TBAA tags for pointers with different depths,types. (PR #76612)
@@ -185,10 +185,33 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type
*Ty) {
return getChar();
// Handle pointers and references.
rjmccall wrote:
Probably worth putting standard citations here:
```suggestion
// Handle pointers and references.
//
// C has a very strict rule for pointer aliasing. C23 6.7.6.1p2:
// For two pointer types to be compatible, both shall be identically
// qualified and both shall be pointers to compatible types.
//
// This rule is impractically strict; we want to at least ignore CVR
// qualifiers. Distinguishing by CVR qualifiers would make it UB to
// e.g. cast a `char **` to `const char * const *` and dereference it,
// which is too common and useful to invalidate. C++'s similar types
// rule permits qualifier differences in these nested positions; in fact,
// C++ even allows that cast as an implicit conversion.
//
// Other qualifiers could theoretically be distinguished, especially if
// they involve a significant representation difference. We don't
// currently do so, however.
//
// Computing the pointee type string recursively is implicitly more
// forgiving than the standards require. Effectively, we are turning
// the question "are these types compatible/similar" into "are
// accesses to these types allowed to alias". In both C and C++,
// the latter question has special carve-outs for signedness
// mismatches that only apply at the top level. As a result, we are
// allowing e.g. `int *` l-values to access `unsigned *` objects.
```
https://github.com/llvm/llvm-project/pull/76612
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Emit distinct TBAA tags for pointers with different depths,types. (PR #76612)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/76612 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Emit distinct TBAA tags for pointers with different depths,types. (PR #76612)
https://github.com/rjmccall commented: Generally looking good. https://github.com/llvm/llvm-project/pull/76612 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Emit distinct TBAA tags for pointers with different depths,types. (PR #76612)
@@ -185,10 +185,33 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type
*Ty) {
return getChar();
// Handle pointers and references.
- // TODO: Implement C++'s type "similarity" and consider dis-"similar"
- // pointers distinct.
- if (Ty->isPointerType() || Ty->isReferenceType())
-return createScalarTypeNode("any pointer", getChar(), Size);
+ if (Ty->isPointerType() || Ty->isReferenceType()) {
+llvm::MDNode *AnyPtr = createScalarTypeNode("any pointer", getChar(),
Size);
+if (CodeGenOpts.RelaxedPointerAliasing)
+ return AnyPtr;
+// Compute the depth of the pointer and generate a tag of the form
"p
+// ".
+unsigned PtrDepth = 0;
+do {
+ PtrDepth++;
+ Ty = Ty->getPointeeType().getTypePtr();
+} while (Ty->isPointerType() || Ty->isReferenceType());
rjmccall wrote:
A reference type is impossible in these nested positions; you can never have
references/pointers to references. I believe it is possible in the original
position because we can use this when building aggregate TBAA.
https://github.com/llvm/llvm-project/pull/76612
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
rjmccall wrote: That sounds fine to me as long as we're still emitting projections of them properly (i.e. not just assuming "oh, it's an empty record, we can use whatever pointer we want because it'll never be dereferenced"). https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [ObjC][CodeGen] Assume a for-in loop is in bounds and cannot overflow (PR #94885)
rjmccall wrote: I don't usually do that for people, sorry. https://github.com/llvm/llvm-project/pull/94885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -2012,26 +2015,27 @@ llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address
Addr, bool Volatile,
}
llvm::Value *CodeGenFunction::EmitToMemory(llvm::Value *Value, QualType Ty) {
- // Bool has a different representation in memory than in registers.
- if (hasBooleanRepresentation(Ty)) {
-// This should really always be an i1, but sometimes it's already
-// an i8, and it's awkward to track those cases down.
-if (Value->getType()->isIntegerTy(1))
- return Builder.CreateZExt(Value, ConvertTypeForMem(Ty), "frombool");
-assert(Value->getType()->isIntegerTy(getContext().getTypeSize(Ty)) &&
- "wrong value rep of bool");
+ if (hasBooleanRepresentation(Ty) ||
+ (Ty->isBitIntType() && Value->getType()->isIntegerTy())) {
rjmccall wrote:
That really shouldn't happen — the precondition on this function should be that
`Value` has the actual scalar type. Whenever you're not seeing that, there's
somewhere else that's doing a raw load that needs to use `EmitLoadOfScalar`
function instead. Certainly that's the case of an indirect return value.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -118,6 +124,37 @@ llvm::Type *CodeGenTypes::ConvertTypeForMem(QualType T,
bool ForBitField) {
return R;
}
+bool CodeGenTypes::LLVMTypeLayoutMatchesAST(QualType ASTTy,
+llvm::Type *LLVMTy) {
+ CharUnits ASTSize = Context.getTypeSizeInChars(ASTTy);
+ CharUnits LLVMSize =
+ CharUnits::fromQuantity(getDataLayout().getTypeAllocSize(LLVMTy));
+ return ASTSize == LLVMSize;
+}
+
+llvm::Type *CodeGenTypes::convertTypeForLoadStore(QualType T,
+ llvm::Type *LLVMTy) {
+ if (!LLVMTy)
+LLVMTy = ConvertType(T);
+
+ if (!T->isBitIntType() && LLVMTy->isIntegerTy(1))
+return llvm::IntegerType::get(getLLVMContext(),
+ (unsigned)Context.getTypeSize(T));
+
+ if (T->isBitIntType()) {
+llvm::Type *R = ConvertType(T);
+if (!LLVMTypeLayoutMatchesAST(T, R))
+ return llvm::Type::getIntNTy(
+ getLLVMContext(), Context.getTypeSizeInChars(T).getQuantity() * 8);
rjmccall wrote:
I would like to use LLVM's `iN` type when it actually has the right data
layout. If we decide to give up and use a Rust-like opaque approach, we should
do it comprehensively and not just for `_BitInt`.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] fix the unexpected controlflow in `ParseTentative.cpp` (PR #95917)
rjmccall wrote: > @rjmccall -- do you have ideas on how we can trigger the issue here or do you > think this code can be removed? I wouldn't be surprised either way — tentative parsing often contains code that feels like it ought to be redundant, but it also often takes strange paths in strange situations. You might try writing `ident` in something like the parameter clause of a local function declaration, where ObjC++ will have to disambiguate it from a direct initialization but where the parser will enter tentative parsing without `ident` being the immediately following token. https://github.com/llvm/llvm-project/pull/95917 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix potential null pointer dereferences in retain cycle detection (PR #95192)
rjmccall wrote: Okay. It looks like it's actually impossible for this to be null — since we're looking at a `super` dispatch, we must be inside an Objective-C method declaration. We do appreciate people running static analysis on our code base, but please think of analysis reports as the start of an investigation rather than something that needs to be reflexively fixed. https://github.com/llvm/llvm-project/pull/95192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix potential null pointer dereferences in retain cycle detection (PR #95192)
https://github.com/rjmccall closed https://github.com/llvm/llvm-project/pull/95192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix potential null pointer dereferences in retain cycle detection (PR #95192)
rjmccall wrote: Do you have a test case? https://github.com/llvm/llvm-project/pull/95192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Lower _BitInt(129+) to a different type in LLVM IR (PR #91364)
@@ -1533,9 +1533,17 @@ void CodeGenFunction::EmitReturnStmt(const ReturnStmt
) {
Builder.CreateStore(Result.getScalarVal(), ReturnValue);
} else {
switch (getEvaluationKind(RV->getType())) {
-case TEK_Scalar:
- Builder.CreateStore(EmitScalarExpr(RV), ReturnValue);
- break;
+case TEK_Scalar: {
+ llvm::Value *Ret = EmitScalarExpr(RV);
+ // EmitStoreOfScalar could be used here, but it extends bool which for
+ // some targets is returned as i1 zeroext.
rjmccall wrote:
I don't think so; we'd need an actual flag in `CodeGenFunction`. Maybe we
could recreate it without that by looking at the value.
https://github.com/llvm/llvm-project/pull/91364
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -325,14 +325,19 @@ Address SparcV9ABIInfo::EmitVAArg(CodeGenFunction , Address VAListAddr, break; case ABIArgInfo::Ignore: -return Address(llvm::UndefValue::get(ArgPtrTy), ArgTy, TypeInfo.Align); +return CGF.EmitLoadOfAnyValue( +CGF.MakeAddrLValue( rjmccall wrote: Yeah, we should probably separately add some function to synthesize an ignored value. https://github.com/llvm/llvm-project/pull/94635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -789,27 +791,37 @@ Address AArch64ABIInfo::EmitAAPCSVAArg(Address
VAListAddr, QualType Ty,
OnStackBlock, "vaargs.addr");
if (IsIndirect)
-return Address(CGF.Builder.CreateLoad(ResAddr, "vaarg.addr"), ElementTy,
- TyAlign);
-
- return ResAddr;
+return CGF.EmitLoadOfAnyValue(
+CGF.MakeAddrLValue(
+Address(CGF.Builder.CreateLoad(ResAddr, "vaarg.addr"), ElementTy,
+TyAlign),
+Ty),
+Slot);
+
+ return CGF.EmitLoadOfAnyValue(CGF.MakeAddrLValue(ResAddr, Ty), Slot);
}
-Address AArch64ABIInfo::EmitDarwinVAArg(Address VAListAddr, QualType Ty,
-CodeGenFunction ) const {
+RValue AArch64ABIInfo::EmitDarwinVAArg(Address VAListAddr, QualType Ty,
+ CodeGenFunction ,
+ AggValueSlot Slot) const {
// The backend's lowering doesn't support va_arg for aggregates or
// illegal vector types. Lower VAArg here for these cases and use
// the LLVM va_arg instruction for everything else.
if (!isAggregateTypeForABI(Ty) && !isIllegalVectorType(Ty))
-return EmitVAArgInstr(CGF, VAListAddr, Ty, ABIArgInfo::getDirect());
+return CGF.EmitLoadOfAnyValue(
+CGF.MakeAddrLValue(
+EmitVAArgInstr(CGF, VAListAddr, Ty, ABIArgInfo::getDirect()), Ty),
+Slot);
uint64_t PointerSize = getTarget().getPointerWidth(LangAS::Default) / 8;
CharUnits SlotSize = CharUnits::fromQuantity(PointerSize);
// Empty records are ignored for parameter passing purposes.
- if (isEmptyRecord(getContext(), Ty, true))
-return Address(CGF.Builder.CreateLoad(VAListAddr, "ap.cur"),
- CGF.ConvertTypeForMem(Ty), SlotSize);
+ if (isEmptyRecord(getContext(), Ty, true)) {
+Address ResAddr = Address(CGF.Builder.CreateLoad(VAListAddr, "ap.cur"),
rjmccall wrote:
It's fine to have an `alloca` that ends up unused. It'd be slightly better not
to, so if you can easily avoid the allocation by e.g. doing the empty-class
check earlier, please do; but if that's awkward to achieve, don't worry about
it.
https://github.com/llvm/llvm-project/pull/94635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -789,27 +791,37 @@ Address AArch64ABIInfo::EmitAAPCSVAArg(Address
VAListAddr, QualType Ty,
OnStackBlock, "vaargs.addr");
if (IsIndirect)
-return Address(CGF.Builder.CreateLoad(ResAddr, "vaarg.addr"), ElementTy,
- TyAlign);
-
- return ResAddr;
+return CGF.EmitLoadOfAnyValue(
+CGF.MakeAddrLValue(
+Address(CGF.Builder.CreateLoad(ResAddr, "vaarg.addr"), ElementTy,
+TyAlign),
+Ty),
+Slot);
+
+ return CGF.EmitLoadOfAnyValue(CGF.MakeAddrLValue(ResAddr, Ty), Slot);
}
-Address AArch64ABIInfo::EmitDarwinVAArg(Address VAListAddr, QualType Ty,
-CodeGenFunction ) const {
+RValue AArch64ABIInfo::EmitDarwinVAArg(Address VAListAddr, QualType Ty,
+ CodeGenFunction ,
+ AggValueSlot Slot) const {
// The backend's lowering doesn't support va_arg for aggregates or
// illegal vector types. Lower VAArg here for these cases and use
// the LLVM va_arg instruction for everything else.
if (!isAggregateTypeForABI(Ty) && !isIllegalVectorType(Ty))
-return EmitVAArgInstr(CGF, VAListAddr, Ty, ABIArgInfo::getDirect());
+return CGF.EmitLoadOfAnyValue(
+CGF.MakeAddrLValue(
+EmitVAArgInstr(CGF, VAListAddr, Ty, ABIArgInfo::getDirect()), Ty),
+Slot);
uint64_t PointerSize = getTarget().getPointerWidth(LangAS::Default) / 8;
CharUnits SlotSize = CharUnits::fromQuantity(PointerSize);
// Empty records are ignored for parameter passing purposes.
- if (isEmptyRecord(getContext(), Ty, true))
-return Address(CGF.Builder.CreateLoad(VAListAddr, "ap.cur"),
- CGF.ConvertTypeForMem(Ty), SlotSize);
+ if (isEmptyRecord(getContext(), Ty, true)) {
+Address ResAddr = Address(CGF.Builder.CreateLoad(VAListAddr, "ap.cur"),
rjmccall wrote:
Yeah, for this and the point above, it feels like we should be able to just do
nothing. (`Ignore` can only be used for empty types.)
https://github.com/llvm/llvm-project/pull/94635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
https://github.com/rjmccall commented: Other than the one slight nit (that I think may be irrelevant), this LGTM. I'd like @efriedma-quic to also do a quick review if he has time, though. https://github.com/llvm/llvm-project/pull/94635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -2161,6 +2161,21 @@ static RValue EmitLoadOfMatrixLValue(LValue LV,
SourceLocation Loc,
return RValue::get(CGF.EmitLoadOfScalar(LV, Loc));
}
+RValue CodeGenFunction::EmitLoadOfAnyValue(LValue LV, AggValueSlot Slot,
+ SourceLocation Loc) {
+ QualType Ty = LV.getType();
+ switch (getEvaluationKind(Ty)) {
+ case TEK_Scalar:
+return EmitLoadOfLValue(LV, Loc);
+ case TEK_Complex:
+return RValue::getComplex(EmitLoadOfComplex(LV, Loc));
+ case TEK_Aggregate:
+EmitAggFinalDestCopy(Ty, Slot, LV, EVK_RValue);
rjmccall wrote:
I believe the EVK argument here is mean to represent the kind of value that the
l-value represents. In that case, it's an l-value — we need to preserve the
underlying `va_list` storage and not treat it as something we own, because it's
possible to iterate varargs twice. I'm not sure it makes a practical
difference here because variadic argumets are required to be trivial, but it's
better to be accurate.
https://github.com/llvm/llvm-project/pull/94635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/94635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
https://github.com/rjmccall edited https://github.com/llvm/llvm-project/pull/94635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -2161,6 +2161,19 @@ static RValue EmitLoadOfMatrixLValue(LValue LV,
SourceLocation Loc,
return RValue::get(CGF.EmitLoadOfScalar(LV, Loc));
}
+RValue CodeGenFunction::EmitLoadOfAnyValue(LValue LV, SourceLocation Loc) {
+ QualType Ty = LV.getType();
+ switch (getEvaluationKind(Ty)) {
+ case TEK_Scalar:
+return EmitLoadOfLValue(LV, Loc);
+ case TEK_Complex:
+return RValue::getComplex(EmitLoadOfComplex(LV, Loc));
+ case TEK_Aggregate:
+return RValue::getAggregate(LV.getAddress());
rjmccall wrote:
I'm concerned about this laundering an l-value address into an aggregate
r-value address. If someone used this as a generic routine somewhere else in
CodeGen, this could pretty easily cause subtle miscompiles. Can we have the
AggExprEmitter pass the `AggValueSlot` to `EmitVAArg` and so on, so that it
eventually gets passed to this method? And then this method can call
`EmitAggFinalDestCopy`.
It's probably okay for our existing use case here, though, where we're
That might be okay for our specific use case here — we probably never mutate
the aggregate slot we get back during aggregate expression evaluation, and the
slot in the `va_list` should otherwise be
https://github.com/llvm/llvm-project/pull/94635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [ObjC][CodeGen] Assume a for-in loop is in bounds and cannot overflow (PR #94885)
https://github.com/rjmccall commented: LGTM. Do we not have any tests that actually test the add? https://github.com/llvm/llvm-project/pull/94885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CodeGen] Simplify codegen for array initialization (PR #93956)
https://github.com/rjmccall approved this pull request. Sure, LGTM. https://github.com/llvm/llvm-project/pull/93956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -1328,15 +1328,15 @@ void AggExprEmitter::VisitChooseExpr(const ChooseExpr
*CE) {
void AggExprEmitter::VisitVAArgExpr(VAArgExpr *VE) {
Address ArgValue = Address::invalid();
- Address ArgPtr = CGF.EmitVAArg(VE, ArgValue);
+ RValue ArgPtr = CGF.EmitVAArg(VE, ArgValue);
// If EmitVAArg fails, emit an error.
- if (!ArgPtr.isValid()) {
+ if (!ArgValue.isValid()) {
CGF.ErrorUnsupported(VE, "aggregate va_arg expression");
return;
}
- EmitFinalDestCopy(VE->getType(), CGF.MakeAddrLValue(ArgPtr, VE->getType()));
+ EmitFinalDestCopy(VE->getType(), ArgPtr);
rjmccall wrote:
CodeGen handles aggregates by keeping them in memory. Everything to do with
aggregate expression emission generally works by passing around a destination
address that the value should be placed in — you evaluate the expression into
that address, then just put that address into the `RValue` you return. If you
don't pass an address down, generally operations have to evaluate into a
temporary, which can create redundant `memcpy`s.
In `AggExprEmitter`, the destination address is the `Dest` member. We may just
be doing an assignment into `Dest` and not an initialization, though, which has
important semantic differences in some cases and is (confusingly, sorry)
tracked with `.isPotentiallyAliased()`. The easiest way to handle that is to
make sure the final copy is done with `EmitFinalDestCopy`. You'll need to add
a method like this to `CodeGenFunction`:
```c++
void EmitAggFinalDestCopy(QualType type, AggValueSlot dest, const LValue ,
ExprValueKind srcKind);
```
which makes an `AggExprEmitter` and calls `EmitFinalDestCopy` on it.
https://github.com/llvm/llvm-project/pull/94635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
rjmccall wrote: > > I was hoping that you might push this down into the target code — if you > > make emitVoidPtrVAArg return an RValue, that'll handle about half of the > > targets. Most of the rest will just need an EmitLoadOfLValue at the end > > Ok, that seems doable, but I'm having trouble with `EmitLoadOfLValue`. Oh, sorry, I should have looked closer — I didn't notice that this only handles the scalar case. You should probably still call it instead of just emitting your own load for scalars, but yeah, you'll need to switch over the evaluation kind and then call either `EmitLoadOfLValue`, `EmitLoadOfComplex`, or `EmitAggregateCopy`. > In case target type is an aggregate, `EmitLoadOfLValue` still loads it as a > scalar, so later `EmitFinalDestCopy` fails with an assertion because returned > RValue is not aggregate. I could just return `RValue::getAggregate` when > target type is aggregate but I will have to check each time (about 16 places > where I inserted `EmitLoadOfLValue`, I suppose) which makes it fairly big > amount of code. Am I doing something wrong? Only to the extent that I mislead you. You can make a common function to call to do this, though. https://github.com/llvm/llvm-project/pull/94635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
https://github.com/rjmccall commented: I was hoping that you might push this down into the target code — if you make `emitVoidPtrVAArg` return an `RValue`, that'll handle about half of the targets. Most of the rest will just need an `EmitLoadOfLValue` at the end. MIPS (which is the target with the weird unpromotion logic) calls `emitVoidPtrVAArg`, so it will just need to do its unpromotion on the scalar value, which it should be able to do without creating a temporary. But this works as a first pass if you think that's too much to ask. https://github.com/llvm/llvm-project/pull/94635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -5988,12 +5988,29 @@ CGCallee
CGCallee::prepareConcreteCallee(CodeGenFunction ) const {
/* VarArg handling */
-Address CodeGenFunction::EmitVAArg(VAArgExpr *VE, Address ) {
- VAListAddr = VE->isMicrosoftABI()
- ? EmitMSVAListRef(VE->getSubExpr())
- : EmitVAListRef(VE->getSubExpr());
+RValue CodeGenFunction::EmitVAArg(VAArgExpr *VE, Address ) {
+ VAListAddr = VE->isMicrosoftABI() ? EmitMSVAListRef(VE->getSubExpr())
+: EmitVAListRef(VE->getSubExpr());
QualType Ty = VE->getType();
+ Address ResAddr = Address::invalid();
if (VE->isMicrosoftABI())
-return CGM.getTypes().getABIInfo().EmitMSVAArg(*this, VAListAddr, Ty);
- return CGM.getTypes().getABIInfo().EmitVAArg(*this, VAListAddr, Ty);
+ResAddr = CGM.getTypes().getABIInfo().EmitMSVAArg(*this, VAListAddr, Ty);
+ else
+ResAddr = CGM.getTypes().getABIInfo().EmitVAArg(*this, VAListAddr, Ty);
+
+ switch (getEvaluationKind(VE->getType())) {
+ case TEK_Scalar: {
+llvm::Value *Load = Builder.CreateLoad(ResAddr);
+return RValue::get(Load);
+ }
+ case TEK_Complex: {
+llvm::Value *Load = Builder.CreateLoad(ResAddr);
+llvm::Value *Real = Builder.CreateExtractValue(Load, 0);
+llvm::Value *Imag = Builder.CreateExtractValue(Load, 1);
+return RValue::getComplex(std::make_pair(Real, Imag));
rjmccall wrote:
There's an `EmitLoadOfComplex` you can use here. You'll just need to
`MakeAddrLValue` first.
https://github.com/llvm/llvm-project/pull/94635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CodeGen] Return RValue from `EmitVAArg` (PR #94635)
@@ -1328,15 +1328,15 @@ void AggExprEmitter::VisitChooseExpr(const ChooseExpr
*CE) {
void AggExprEmitter::VisitVAArgExpr(VAArgExpr *VE) {
Address ArgValue = Address::invalid();
- Address ArgPtr = CGF.EmitVAArg(VE, ArgValue);
+ RValue ArgPtr = CGF.EmitVAArg(VE, ArgValue);
// If EmitVAArg fails, emit an error.
- if (!ArgPtr.isValid()) {
+ if (!ArgValue.isValid()) {
CGF.ErrorUnsupported(VE, "aggregate va_arg expression");
return;
}
- EmitFinalDestCopy(VE->getType(), CGF.MakeAddrLValue(ArgPtr, VE->getType()));
+ EmitFinalDestCopy(VE->getType(), ArgPtr);
rjmccall wrote:
It would be more standard here to pass the dest slot down.
https://github.com/llvm/llvm-project/pull/94635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
