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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2077
+        getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy)))
+      SRETAttrs.addAlignmentAttr(Align);
     ArgAttrs[IRFunctionArgs.getSRetArgNo()] =
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74183/new/

https://reviews.llvm.org/D74183