jdoerfert added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:2082
+ const Type *RetTyPtr = RetTy.getTypePtr();
+ if (!RetTy->isVoidType() && !RetTyPtr->isRecordType() &&
+ RetAI.getKind() != ABIArgInfo::Indirect) {
----------------
guiand wrote:
> guiand wrote:
> > guiand wrote:
> > > rsmith wrote:
> > > > Other types that we should think about from a padding perspective:
> > > >
> > > > * `nullptr_t` (which has the same size and alignment as `void*` but is
> > > > all padding)
> > > > * 80-bit `long double` and `_Complex long double` (I don't know
> > > > whether we guarantee to initialize the 6 padding bytes)
> > > > * member pointers might contain padding under some ABI rules -- under
> > > > the MS ABI, you get a struct containing N pointers followed by M ints,
> > > > which could have 4 bytes of tail padding on 64-bit targets
> > > > * vector types with tail padding (eg, a vector of 3 `char`s is
> > > > sometimes passed as an `i32` with one byte `undef`)
> > > > * matrix types (presumably the same concerns as for vector types apply
> > > > here)
> > > > * maybe Obj-C object types?
> > > > * `_ExtInt` types (eg, returning an `_ExtInt(65)` initialized to 0
> > > > produces an `{i64, i64}` containing 7 bytes of `undef`)
> > > >
> > > > It would be safer to list exactly those types for which we know this
> > > > assumption is correct rather than assuming that it's correct by default
> > > > -- I think more than half of the `Type` subclasses for concrete
> > > > canonical types can have undef bits in their IR representation.
> > > This is a good point, and I think there was some nuance that I had
> > > previously included in determining this for records (since removed) that
> > > I didn't think to include in CGCall.cpp.
> > >
> > > I think one particular check,
> > > `llvm::DataLayout::typeSizeEqualsStoreSize`, handles a lot of these cases:
> > > - `long double`
> > > - oddly-sized vectors
> > > - `_ExtInt` types
> > >
> > > I don't know if the `matrix` type has internal padding (like structs) but
> > > if not it would cover that as well.
> > > I believe that the struct with padding wouldn't be an issue as we're
> > > excluding record types here.
> > > And I'd have to take a closer look at `nullptr_t` to see how it behaves
> > > here.
> > >
> > > ~~~
> > >
> > > But if I'd like to build an exhaustive list of types I think will work
> > > correctly with a check like this:
> > > - scalars
> > > - floating point numbers
> > > - pointers
> > >
> > > I think I'd need also need an inner `typeSizeEqualsStoreSize` check on
> > > the base type of vectors/complex numbers.
> > > nullptr_t (which has the same size and alignment as void* but is all
> > > padding)
> >
> > From what I can gather looking at the IR of `test/CodeGenCXX/nullptr.cpp`,
> > it looks like `nullptr_t` arguments and return values are represented as
> > normal `i8*` types but given the special value `null`. From this it seems
> > like we can mark them `noundef` without worry?
> Actually I've been thinking more about these cases, and they should only
> really be a problem if clang coerces them away from their native types to
> something larger.
>
> For example, if clang emits `i33` that's not a problem, as potential padding
> belonging to `i33` can be always be determined by looking at the data layout.
> But if it emits `{ i32, i32 }` or `i64` or something similar, it's
> introducing new hidden/erased padding and so we can't mark it `noundef`.
> Same thing goes for `long double`: if it's emitted as `x86_fp80` that's no
> problem.
>
> There's been some confusion wrt this in the LangRef patch as well. I think
> what we really want is an attribute that indicates the presence of undef bits
> *not inherent to the IR type*. So (for the sake of argument, we're not
> handling structs right now) `struct { char; short }` emitted as `{ i8, i16 }
> noundef` is fine, but emitting it as `i64` is a problem.
> struct { char; short } emitted as { i8, i16 } noundef is fine, but emitting
> it as i64 is a problem.
FWIW, I agree with that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81678/new/
https://reviews.llvm.org/D81678
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
