[PATCH] D54653: Correct CreateAlignmentAssumption to check sign and power-of-2 (LLVM Part)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:2200
+
+    // Alignments are only valid if  > 0 && IsPowerOf2.
+    Value *IsPositive = CreateICmp(
----------------
erichkeane wrote:
> hfinkel wrote:
> > True, but I don't see why we're checking it here. It's just UB.
> @jyknight encouraged this patch on IRC.  I believe it makes @lebedev.ri s job on the sanitizer easier.
These patches do not affect the sanitizer patches in any way.
Using correct predicate will simply mean that you will not erroneously discard alignment request of `2147483648U` (sign bit).



================
Comment at: include/llvm/IR/IRBuilder.h:2201-2203
+    Value *IsPositive = CreateICmp(
+        Sign == AlignmentSign::Signed ? CmpInst::ICMP_SGT : CmpInst::ICMP_UGT,
+        Alignment, ConstantInt::get(Alignment->getType(), 0), "ispositive");
----------------
erichkeane wrote:
> lebedev.ri wrote:
> > Would it be better to only keep the predicate change, and leave the power-of-two as UB?
> We already have that before this change, though it has a mis-signedness.  However, there isn't really issue treating it as signed always, since values outside of the range of a signed value are illegal/UB anyway.  IMO, this would be the 'don't change things' opinion.
Replied to the wrong comment maybe?
The question was - do we really want to do `@llvm.assume()` if the requested alignment
is really power-of-two at the run time? That will somewhat confuse the middle-end,
and is UB in any case, as @hfinkel said.


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

https://reviews.llvm.org/D54653