dexonsmith added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:2077
+ getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy)))
+ SRETAttrs.addAlignmentAttr(Align);
ArgAttrs[IRFunctionArgs.getSRetArgNo()] =
----------------
rjmccall wrote:
> 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.
> I think we're going to end up needing an alignment there in all cases
> eventually because of opaque pointer types.
That's a great point. In that case I don't have a strong opinion.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74183/new/
https://reviews.llvm.org/D74183
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
