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

Hal Finkel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 3 17:23:56 PST 2018


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:
> hfinkel wrote:
> > 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.
> > 
> I agree, we shouldn't introduce multiple basic blocks here. This patch doesn't seem to, however. That is, I think what's here seems good -- not creating additional UB, and not introducing new basic blocks.
> 
> Perhaps I'm misunderstanding something?
This is still adding the IsPowerOf2 check, and I don't see why that's necessary. If it's not a power of 2, that's UB. Also, that's a separate change from allowing the caller to choose whether to treat the alignment as signed or unsigned.

To be fair and consistent, I'm not sure why we're checking IsPositive either. It looks like @erichkeane added that in r298643 (D29598). I don't see any discussion of this during the review, but it seems also somewhat unfortunate. If the formula used for the alignment assumptions are in any way non-trivial, then utilities like computeKnownBits are far less likely to say useful things based on them.



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

https://reviews.llvm.org/D54653





More information about the llvm-commits mailing list