jfb added a comment.
Thanks for the detailed comments, I think I've addressed all of them! I also
added UBSan support to check the builtin invocation. I think this patch is
pretty much ready to go. A follow-up will need to add the support functions to
compiler-rt (they're currently optional, as per
https://reviews.llvm.org/D33240), and in cases where size is known we can
inline them (as we do for memcpy and friends).
================
Comment at: clang/docs/LanguageExtensions.rst:2454
+and might be non-uniform throughout the operation.
+
+The builtins can be used as building blocks for different facilities:
----------------
rsmith wrote:
> From the above description, I think the documentation is unclear what the
> types `T` and `U` are used for. I think the answer is something like:
>
> """
> The types `T` and `U` are required to be trivially-copyable types, and
> `byte_element_size` (if specified) must be a multiple of the size of both
> types. `dst` and `src` are expected to be suitably aligned for `T` and `U`
> objects, respectively.
> """
>
> But... we actually get the alignment information by analyzing pointer
> argument rather than from the types, just like we do for `memcpy` and
> `memmove`, so maybe the latter part is not right. (What did you intend
> regarding alignment for the non-atomic case?) The trivial-copyability and
> divisibility checks don't seem fundamentally important to the goal of the
> builtin, so I wonder if we could actually just use `void` here and remove the
> extra checks. (I don't really have strong views one way or the other on this,
> except that we should either document what `T` and `U` are used for or make
> the builtins not care about the pointee type beyond its qualifiers.)
You're right. I've removed most treatment of `T` / `U`, and updated the
documentation.
I left the trivial copy check, but `void*` is a usual escape hatch.
Divisibility is now only checked for `size` / `element_size`.
================
Comment at: clang/lib/AST/ExprConstant.cpp:8851
+ // about atomicity, but needs to check runtime constraints on size. We
+ // can't check the alignment runtime constraints.
+ APSInt ElSz;
----------------
rsmith wrote:
> We could use the same logic we use in `__builtin_is_aligned` here. For any
> object whose value the constant evaluator can reason about, we should be able
> to compute at least a minimal alignment (though the actual runtime alignment
> might of course be greater).
I think the runtime alignment is really the only thing that matters here. I
played with constexpr checking based on what `__builtin_is_aligned` does, and
it's not particularly useful IMO.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2639-2640
case Builtin::BI__builtin_mempcpy: {
+ QualType DestTy = getPtrArgType(CGM, E, 0);
+ QualType SrcTy = getPtrArgType(CGM, E, 1);
Address Dest = EmitPointerWithAlignment(E->getArg(0));
----------------
rsmith wrote:
> Looking through implicit conversions in `getPtrArgType` here will change the
> code we generate for cases like:
>
> ```
> void f(volatile void *p, volatile void *q) {
> memcpy(p, q, 4);
> }
> ```
>
> ... (in C, where we permit such implicit conversions) to use a volatile
> memcpy intrinsic. Is that an intentional change?
I'm confused... what's the difference that this makes for the pre-existing
builtins? My intent was to get the `QualType` unconditionally, but I can
conditionalize it if needed... However this ought to make no difference:
```
static QualType getPtrArgType(CodeGenModule &CGM, const CallExpr *E,
unsigned ArgNo) {
QualType ArgTy = E->getArg(ArgNo)->IgnoreImpCasts()->getType();
if (ArgTy->isArrayType())
return CGM.getContext().getAsArrayType(ArgTy)->getElementType();
if (ArgTy->isObjCObjectPointerType())
return ArgTy->castAs<clang::ObjCObjectPointerType>()->getPointeeType();
return ArgTy->castAs<clang::PointerType>()->getPointeeType();
}
```
and indeed I can't see the example you provided change in IR from one to the
other. The issue I'm working around is that getting it unconditionally would
make ObjC code sad when `id` is passed in as I outlined above.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:5567
+ << (int)ElSz->getLimitedValue() << DstElSz << DstValTy
+ << DstOp->getSourceRange() << Arg->getSourceRange());
+
----------------
jfb wrote:
> I'm re-thinking these checks:
> ```
> if (ElSz->urem(DstElSz))
> return ExprError(
> Diag(TheCall->getBeginLoc(),
> PDiag(diag::err_atomic_builtin_ext_size_mismatches_el))
> << (int)ElSz->getLimitedValue() << DstElSz << DstValTy
> << DstOp->getSourceRange() << Arg->getSourceRange());
> ```
> I'm not sure we ought to have them anymore. We know that the types are
> trivially copyable, it therefore doesn't really matter if you're copying with
> operations smaller than the type itself. For example:
> ```
> struct Data {
> int a, b, c, d;
> };
> ```
> It ought to be fine to do 4-byte copies of `Data`, if whatever your algorithm
> is is happy with that. I therefore think I'll remove these checks based on
> the dst / src element types. The only thing that seems to make sense is
> making sure that you don't straddle object boundaries with element size.
>
> I removed sizeless types: we'll codegen whatever you ask for.
They're gone, we now only check that size and element size match up.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79279/new/
https://reviews.llvm.org/D79279
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
