[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

Fri Oct 11 06:12:57 PDT 2019

erichkeane marked 5 inline comments as done.
erichkeane added subscribers: majnemer, aaron.ballman.
erichkeane added a comment.

In D68824#1704942 <https://reviews.llvm.org/D68824#1704942>, @rsmith wrote:

> This seems to be missing a CodeGen test for what IR we generate on an overly-large alignment. (The warning says the alignment is ignored, but I don't see where you're actually doing anything different in that case when generating IR.)

My intent was to just let the value through, and let LLVM ignore it (while alerting the developer).  I'll add a CodeGen test as well when relanding, it seems to have been lost in the process of developing the patch.  Thanks for your attention here.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2857-2859
+    : Warning<"requested alignment must be %0 bytes or smaller; assumption "
+              "ignored">,
+      InGroup<DiagGroup<"builtin-assume-aligned-alignment">>;
----------------
rsmith wrote:
> Ignoring the assumption in this case seems unnecessarily user-hostile (and would require hard-coding an arbitrary LLVM limit in the source code to work around). Couldn't we just assume the highest alignment that we do support instead?
This more closely fit GCCs behavior (of ignoring any invalid value, though their top limit is int64-max).  We can definitely just assume highest alignment, I'll do that in my re-land.

================
Comment at: clang/lib/Sema/SemaChecking.cpp:6067-6070
+    // Alignment calculations can wrap around if it's greater than 2**28.
+    unsigned MaximumAlignment =
+        Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192
+                                                                : 268435456;
----------------
rsmith wrote:
> Why is there a different limit depending on bin format? We can support this at the IR level regardless, can't we? (I don't see how the binary format is relevant.)
I'd copied it from the Sema::AddAlignedAttr implementation, but I cannot seem to figure out the origin of that.  @majnemer added the 2**28 business back in 2015, but @aaron.ballman put the limit of 8192 in here: https://reviews.llvm.org/rL158717#change-N0HH8qtBJv7d
(note it was reverted and relanded).

I don't see sufficient justification in that history now that I've looked back to keep that log in here, so I'll keep us at 2**28.

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

https://reviews.llvm.org/D68824