Diff Detail
Unit Tests
Event Timeline
Thanks for the review. The compiler-rt tests appear to be failing because the clang change not to emit the non-power of two assume bundle doesn't seem to be applied and the tests generate IR that is now being rejected.
e.g.
/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/hwasan/TestCases/Linux/aligned_alloc-alignment.cpp:15:27: warning: requested alignment is not a power of 2 [-Wnon-power-of-two-alignment]
void *p = aligned_alloc(17, 100);
^~
alignment must be a power of 2
call void @llvm.assume(i1 true) [ "align"(i8* %call, i64 17) ], !dbg !19
in function main
fatal error: error in backend: Broken function found, compilation aborted!With both patches applied the tests pass locally. I'm not sure how to link the reviews such the build bots will build with both changes applied.
| llvm/lib/IR/Verifier.cpp | ||
|---|---|---|
| 4668 | I believe this should be Assert(Alignment && Alignment->getValue().isPowerOf2()). That is, we should be requiring the alignment to be a constant. I don't think it makes sense to enforce power of two only for constant alignments, but leave non-constant alignments unconstrained -- and non-constant alignments don't really make sense to me in this context. | |
@jdoerfert wasn't the alignment intentionally been left as not being required to be constant?
Yes. Though, I'm not even sure we emit/use non-constant alignments right now. If we emit them we would use them if they are proven to be constant later.
More elaborate support was eventually planned though. I would not require constants here, I overlooked that earlier.
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.
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.
Marking this as changes requested, because we have to either forbid non-constants, or we have to give up on this approach and say that consuming code needs to expect and handle invalid alignments. I don't think there's any in-between options here.
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.
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.
I don't think the argument against an in-between option is actually something we have to worry about. As mentioned before, the only impact is that we would reject problematic code later rather than never.
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
I am worried we might need to check the alignment requirement at the use site instead after all...
I believe this should be Assert(Alignment && Alignment->getValue().isPowerOf2()). That is, we should be requiring the alignment to be a constant. I don't think it makes sense to enforce power of two only for constant alignments, but leave non-constant alignments unconstrained -- and non-constant alignments don't really make sense to me in this context.