rjmccall added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:2077
+ getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy)))
+ SRETAttrs.addAlignmentAttr(Align);
ArgAttrs[IRFunctionArgs.getSRetArgNo()] =
----------------
rjmccall wrote:
> dexonsmith wrote:
> > scanon wrote:
> > > rjmccall wrote:
> > > > Why only when under-aligned? Just to avoid churning tests? I think we
> > > > should apply this unconditionally.
> > > On mainstream architectures today, there's rarely a benefit to knowing
> > > about higher alignment (e.g. MOVUPS is just as fast as MOVAPS if the
> > > address is actually aligned), so we won't see significant perf wins from
> > > preserving over-alignment in most cases, but it also doesn't cost us
> > > anything AFAICT and could deliver wins in some specific cases (e.g. AVX
> > > on SNB and IVB, where I think we split underaligned 256b stores into two
> > > 128b chunks).
> > >
> > > So, yeah, I think we ought to simply unconditionally add the alignment to
> > > the sret.
> > @rjmccall, are you seeing a reason to add the attribute when the implicit
> > one is correct (neither over-aligned nor under-aligned)? If so, it seems
> > to me like the added noise would make the IR harder to read.
> Well, first, I think we're going to end up needing an alignment there in all
> cases eventually because of opaque pointer types. But I also think it's just
> cleaner and more testable to add the attribute in all cases instead of
> leaving it off when the IR type happens to have the right alignment, which
> can be very sensitive to the target.
In general, I think frontends should *never* be leaving it up to LLVM to infer
alignment based on IR types, and this is part-and-parcel with that.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74183/new/
https://reviews.llvm.org/D74183
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
