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

Tue Dec 4 09:06:58 PST 2018

jyknight 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");
----------------
erichkeane wrote:
> hfinkel wrote:
> > 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.
> > 
> I'm not terribly sure why I added the 'IsPositive' check here in the beginning to be honest, perhaps it was simply because the fixed-version did so.  
> 
> If we think that returning negative numbers to UB is acceptable, I can definitely revert all checking here and just emit the assumption instead.  
> 
> I guess the gist of the question we need to ask is UB vs branches?  That question is perhaps made easier by @lebedev.ri and his efforts to add these alignment assumptions to UBSan.
You say that it's UB -- but based on what? The alloc_align attribute and __builtin_assume_aligned call aren't part of the standard. The former could be attached to any function the user desires. Such function may well decide to define the behavior in the face of the error condition, rather than cause UB.

Of course, we can decide that the attribute should cause the compiler to generate UB in such a case, but that's not preordained. And IMO, we should not generate UB where doing so is unnecessary.

So, that gets back to the question before -- what goes wrong (will it ruin the ability to optimize) if we check the value before making an invalid assumption? I'm still not seeing why it would -- AFAICT, either the alignment will be eventually constant (in which case everything folds away), or else we can't really determine anything at all (in which case there are extra IR instructions, which eventually get dropped).

Also, FWIW, I just checked GCC's behavior, which is as close to a spec as we can get here: it does not create UB from passing an invalid alignment. It just ignores the information (as this patch also causes to be the case).

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

https://reviews.llvm.org/D54653