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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
