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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel 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");
----------------
jyknight wrote:
> erichkeane wrote:
> > 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.
> The point of this attribute is to allow the optimizer to make an assumption about the alignment of the pointer returned by a call such as (but not limited to) aligned_alloc. I believe that in almost all relevant situations where this will at all help optimization, the alignment will (eventually, after inlining and simplification) be a constant. At that point, it doesn't matter how complex the expression is, it all folds away.
> 
> The attribute shouldn't cause the compiler to emit UB, because the function it's attached to may in fact have defined behavior in the face of such an argument (e.g., returning NULL, or using the next higher power-of-2 alignment).
> 
> ... At that point, it doesn't matter how complex the expression is, it all folds away.

I agree that in nearly all cases where we can use this the alignment ends up being constant (although this is not strictly necessary should we know that the number is greater than some value). Nevertheless, it is also desirable to minimize the cost when we can't use the information, and having multiple basic blocks that we can't eliminate until we hit CodeGen will interfere with optimization more than just having some extra instructions in a single basic block.



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

https://reviews.llvm.org/D54653