[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

[John McCall via Phabricator via cfe-commits]

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