[PATCH] D110634: [llvm] Update IR verifier to reject non-power-of-2 alignment assume bundles (PR48713).

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D110634#3038367 <https://reviews.llvm.org/D110634#3038367>, @nikic wrote:

> In D110634#3038259 <https://reviews.llvm.org/D110634#3038259>, @jdoerfert wrote:
>
>> In D110634#3038161 <https://reviews.llvm.org/D110634#3038161>, @nikic wrote:
>>
>>> Well, if we don't require constants here then we can't add this check -- this means that some valid input IR (with non-constant non-power-of-two alignment) could become invalid after folding.
>>
>> That is not wrong but also not new, I'd think. The verifier does local verification and after transformations things may be exposed that were missed before. It is always invalid for someone to put a non-power-of-two alignment somewhere and the only questions is when we identify that and reject it.
>
> You're right, but...
>
>>> Allowing non-constants here doesn't make a great deal of sense to me, in terms of practical usefulness. My understanding was that these operand bundles are supposed to model the corresponding metadata, and that doesn't allow non-constant values either.
>>
>> That is true except that we have alignment source annotation that allows non-constant values. E.g., the alignment of the return is the second call argument to this function. If we don't allow non-constant alignments here we loose the ability to express these source annotations in IR.
>
> ...actually making use of non-constant alignments would still require the frontend to ensure that the dynamic alignment is a power of two, otherwise it may fail verification after folding. I guess there //are// cases where this can indeed be ensured (alignment of the form `1 << dynamic`), but the particular `alloc_align` case here does not fall in that category. If we declared that non-power-of-2 alignments are illegal IR (as opposed to just UB), then clang wouldn't actually be able to emit the dynamic alignment case, because it could not guarantee that the resulting IR is valid.
>
> After looking at the `alloc_align` attribute use-case, I've come around to the idea that non-constant alignments can be useful (e.g. so we don't have to drop the info just because the call is happening through a wrapper). In which case I think we should just give up on this, and keep non-power-of-2 alignment as UB rather than invalid IR. Callers will just have to deal with it. Though possibly there could be a helper that only extracts the constant power of two case from the operand bundle and ignores other cases.

And if someone thinks this isn't fun enough yet, passing invalid alignment (not a power of two) isn't actually UB in C: http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_460


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110634