[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:2201-2203
+    Value *IsPositive = CreateICmp(
+        Sign == AlignmentSign::Signed ? CmpInst::ICMP_SGT : CmpInst::ICMP_UGT,
+        Alignment, ConstantInt::get(Alignment->getType(), 0), "ispositive");
----------------
lebedev.ri wrote:
> 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.
err

The question was - do we really want to ONLY 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