[PATCH] D41944: [LLVM][IR][LIT] support of 'no-overflow' flag for sdiv\udiv instructions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:485
 
+  /// \brief Return true if the target support div that may overflow\
+  /// divide by zero without causing a side effect
----------------
Remove slash from end of line. Doxygen comment don't need the new line escaped.


================
Comment at: include/llvm/IR/IRBuilder.h:964
+        return Insert(Folder.CreateUDiv(LC, RC, isExact, isNoOverflow), Name);
+    if (!isExact && !isNoOverflow)
       return Insert(BinaryOperator::CreateUDiv(LHS, RHS), Name);
----------------
Can we just call BinaryOperator::Create(Instruction::UDiv, LHS, RHS) and then just call setIsNoOverflow and setIsExact on the result when needed? This would be similar to the CreateInsertNUWNSWBinOp private helper we have for NSW/NUW. You could make a new helper to be shared by sdiv/udiv.


================
Comment at: lib/AsmParser/LLParser.cpp:3195
+      if (Opc == Instruction::SDiv || Opc == Instruction::UDiv)
+        if (EatIfPresent(lltok::kw_nof))
+          NOF = true;
----------------
This only supports one order for the two keywords. I think you need to support both orders. See the nsw/nuw handling above.


================
Comment at: lib/AsmParser/LLParser.cpp:5189
+    if (Token == lltok::kw_sdiv || Token == lltok::kw_udiv)
+      NOF = EatIfPresent(lltok::kw_nof);
 
----------------
Need to support keywords being in the other order here too.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:573
 
+/// replace the upsupported division with a scalar safe code
+bool CodeGenPrepare::fixMayOverflowIntegerDiv(Instruction &I, bool isSigned) {
----------------
upsupported->unsupported


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:595
+  if (!I.getType()->isVectorTy()) {
+    Value *Result = UndefVal;
+    // %a = sdiv Ty %b, %c ; may overflow or div by zero
----------------
Why is this initialized to Undef? Can't you just declare this where it's assigned below?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:601
+    // BB.cond:
+    //   %cmp = checkSafeValues(%b, %c)
+    //   br %cmp, %BB.true, %BB.merge
----------------
Can you write comments for what's happening in the signed case.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:612
+      Value *CmpPart2 = Builder.CreateICmp(ICmpInst::ICMP_NE, C,
+                                           ConstantInt::get(C->getType(), -1));
+      Value *CmpPart3 = Builder.CreateICmp(
----------------
Should this be ConstantInt::getSigned? If the scalar type is larger than 64-bits that -1 will be padded with zeros by default.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:617
+              B->getType(),
+              APInt::getSignedMinValue(B->getType()->getIntegerBitWidth())));
+      Value *OverflowCheck = Builder.CreateOr(CmpPart2, CmpPart3);
----------------
Can you load the APInt into a temporary variable to shorten this line? It's pretty awful to read right now.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:645
+
+  // vector case
+  Value *VResult = UndefVal;
----------------
This could use some comments showing what the resulting IR should look like.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:647
+  Value *VResult = UndefVal;
+  Value *PrevPhi = UndefVal;
+  unsigned VectorWidth = I.getType()->getVectorNumElements();
----------------
Is the starting value of PrevPhi ever expected to be used? Does it need to be undef?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:662
+  }
+  Value *ZeroVector = ConstantVector::get(ZeroConstants);
+  Value *MinusOneVector = ConstantVector::get(MinusOneConstants);
----------------
If I'm reading this right we're create smallvectors contraining the same element repeatedly? Can we just use ConstantVector::getSplat which will take care of repeating an element?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:682
+
+    Value *CmpIdx = Builder.CreateExtractElement(Cmp, Builder.getInt32(Idx));
+    CondBlock = IfBlock->splitBasicBlock(InsertPt->getIterator(), "cond.div");
----------------
IRBuilder has a CreateExtractElement that takes a uint64_t. You don't need to call getInt32. It will call getInt64 internally.


================
Comment at: lib/IR/AsmWriter.cpp:1150
       Out << " inbounds";
+  } else if (const PossiblyOverflowOperator *PO =
+               dyn_cast<PossiblyOverflowOperator>(U)) {
----------------
You can use "const auto *PO = dyn_cast..."  the type is spelled out in the dyn_cast so we don't need to repeat it


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:933
   Value *RI = Builder.CreateBinOp(I->getOpcode(), Op0, Op1, "phitmp");
   auto *FPInst = dyn_cast<Instruction>(RI);
+  if (FPInst && isa<PossiblyOverflowOperator>(RI)) 
----------------
Can you rename this variable to not say "FP"? I think the FP part was always speculative. It wasn' know FP until the isa<FPMathOperator> was called. But now it looks really confusing to have a variable named FPInst and we're checking a property that could only be set on an integer division.


https://reviews.llvm.org/D41944