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

[Erich Keane via Phabricator via llvm-commits]

erichkeane marked an inline comment as done.
erichkeane 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:
> 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.
Perhaps I'm missing something (it IS monday :) ), but I would presume that the cost of a branch/more BBs to determine that (rather than a select) would be more harmful than just doing a no-op assume.


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

https://reviews.llvm.org/D54653