[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:58
+
+enum class AlignmentSign { Signed, Unsigned};
+
----------------
hfinkel wrote:
> Why not just use a Boolean?
CreateAlignmentAssumption has a default pointer parameter that would convert to boolean.  Additionally, I'm becoming quite militantly against boolean parameters lately :) 


================
Comment at: include/llvm/IR/IRBuilder.h:2200
+
+    // Alignments are only valid if  > 0 && IsPowerOf2.
+    Value *IsPositive = CreateICmp(
----------------
hfinkel wrote:
> True, but I don't see why we're checking it here. It's just UB.
@jyknight encouraged this patch on IRC.  I believe it makes @lebedev.ri s job on the sanitizer easier.


================
Comment at: include/llvm/IR/IRBuilder.h:2213
+    Value *IsPowerOf2 =
+        CreateICmpEQ(AlignAndMinus1, ConstantInt::get(Alignment->getType(), 0),
+                     "ispowertwo");
----------------
craig.topper wrote:
> is Alignment->getType() and IntPtrTy the same thing here? I ask because the Constant in Sub1 was created with one and the ICmp was created with the other.
I believe they are the same (we validate it on 2195, and 'fix' it on 2196), but I can see why it casues confusion.  I'll change them all to Alignment->getType() for clarity's sake.


================
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:
> 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.


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

https://reviews.llvm.org/D54653