[Analysis] Improve EmitGEPOffset by avoiding summ with zero

[yrouban]

Summary

EmitGEPOffset() is improved to avoid generation unneeded operation add 0, Result and return Result instead.

[jdoerfert]

Drive by: Tests needed, commit message missing, and we should see if we can avoid 3-5 line assignments.

[yrouban]

In D90286#2358263, @jdoerfert wrote:

Drive by: Tests needed, commit message missing, and we should see if we can avoid 3-5 line assignments.

EmitGEPOffset() is used by instcombine and we do not see the redundant add 0, X instruction because it is immediately optimized out in the same iteration. It prevents a simple lit test for this patch. On the other hand you can see the fix in InstCombinerImpl::OptimizePointerDifference() which changes the code to expect X in place of add 0, X and there is a related test (test/Transforms/InstCombine/sub-gep.ll), which I believe is enough for this small improvement.
Keep in mind that EmitGEPOffset() could be used in other custom passes that might not require subsequent instcombine to run.

I believe we do not have to adjust sources to make them look better being formatted by the clang formatter. We should improve the formatter instead. But this should not prevent this patch to be accepted.
I'm ok to rename the variables for a better look if this is the only thing that is needed for LGTM, though.

[jdoerfert]

and there is a related test (test/Transforms/InstCombine/sub-gep.ll), which I believe is enough for this small improvement.

Is this test changed? It seems to be missing from the diff.

Keep in mind that EmitGEPOffset() could be used in other custom passes that might not require subsequent instcombine to run.

Noted.

I believe we do not have to adjust sources to make them look better being formatted by the clang formatter. We should improve the formatter instead. But this should not prevent this patch to be accepted. I'm ok to rename the variables for a better look if this is the only thing that is needed for LGTM, though.

Let me rephrase. Please avoid assignments over 3-5 lines, it is hard to read.

[yrouban]

Not needed after NFC https://github.com/llvm/llvm-project/commit/02dda1c by @nikic

llvm/include/llvm/Analysis/Utils/Local.h

 /// pointer). Return the result as a signed integer of intptr size.
 /// When NoAssumptions is true, no assumptions about index computation not
 /// overflowing is made.
 template <typename IRBuilderTy>
 Value *EmitGEPOffset(IRBuilderTy *Builder, const DataLayout &DL, User *GEP,
                      bool NoAssumptions = false) {
   GEPOperator *GEPOp = cast<GEPOperator>(GEP);
   Type *IntIdxTy = DL.getIndexType(GEP->getType());
-  Value *Result = Constant::getNullValue(IntIdxTy);
+  Value *Result = nullptr; // nullptr stands for zero.
 
   // If the GEP is inbounds, we know that none of the addressing operations will
   // overflow in a signed sense.
   bool isInBounds = GEPOp->isInBounds() && !NoAssumptions;
 
   // Build a mask for high order bits.
   unsigned IntPtrWidth = IntIdxTy->getScalarType()->getIntegerBitWidth();
   uint64_t PtrSizeMask =
if (Constant *OpC = dyn_cast<Constant>(Op)) {
         continue;
 
       // Handle a struct index, which adds its field offset to the pointer.
       if (StructType *STy = GTI.getStructTypeOrNull()) {
         uint64_t OpValue = OpC->getUniqueInteger().getZExtValue();
         Size = DL.getStructLayout(STy)->getElementOffset(OpValue);
 
         if (Size)
-          Result = Builder->CreateAdd(Result, ConstantInt::get(IntIdxTy, Size),
-                                      GEP->getName().str()+".offs");
+          Result =
+              !Result
+                  ? ConstantInt::get(IntIdxTy, Size)
+                  : Builder->CreateAdd(Result, ConstantInt::get(IntIdxTy, Size),
+                                       GEP->getName().str() + ".offs");
         continue;
       }
 
       // Splat the constant if needed.
       if (IntIdxTy->isVectorTy() && !OpC->getType()->isVectorTy())
         OpC = ConstantVector::getSplat(
             cast<VectorType>(IntIdxTy)->getElementCount(), OpC);
 
       Constant *Scale = ConstantInt::get(IntIdxTy, Size);
       Constant *OC = ConstantExpr::getIntegerCast(OpC, IntIdxTy, true /*SExt*/);
       Scale =
           ConstantExpr::getMul(OC, Scale, false /*NUW*/, isInBounds /*NSW*/);
       // Emit an add instruction.
-      Result = Builder->CreateAdd(Result, Scale, GEP->getName().str()+".offs");
+      Result = !Result ? Scale
+                       : Builder->CreateAdd(Result, Scale,
+                                            GEP->getName().str() + ".offs");
       continue;
     }
 
     // Splat the index if needed.
     if (IntIdxTy->isVectorTy() && !Op->getType()->isVectorTy())
       Op = Builder->CreateVectorSplat(
           cast<FixedVectorType>(IntIdxTy)->getNumElements(), Op);
 
     // Convert to correct type.
     if (Op->getType() != IntIdxTy)
       Op = Builder->CreateIntCast(Op, IntIdxTy, true, Op->getName().str()+".c");
     if (Size != 1) {
       // We'll let instcombine(mul) convert this to a shl if possible.
       Op = Builder->CreateMul(Op, ConstantInt::get(IntIdxTy, Size),
                               GEP->getName().str() + ".idx", false /*NUW*/,
                               isInBounds /*NSW*/);
     }
 
     // Emit an add instruction.
-    Result = Builder->CreateAdd(Op, Result, GEP->getName().str()+".offs");
+    Result = !Result ? Op
+                     : Builder->CreateAdd(Op, Result,
+                                          GEP->getName().str() + ".offs");
   }
-  return Result;
+
+  return Result ? Result : Constant::getNullValue(IntIdxTy);
 }
 
 }
 
 #endif // LLVM_TRANSFORMS_UTILS_LOCAL_H

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

if (NumNonConstantIndices1 + NumNonConstantIndices2 > 1 &&
       return nullptr;
     }
   }
 
   // Emit the offset of the GEP and an intptr_t.
   Value *Result = EmitGEPOffset(GEP1);
 
   // If this is a single inbounds GEP and the original sub was nuw,
-  // then the final multiplication is also nuw. We match an extra add zero
-  // here, because that's what EmitGEPOffset() generates.
+  // then the final multiplication is also nuw.
   Instruction *I;
   if (IsNUW && !GEP2 && !Swapped && GEP1->isInBounds() &&
-      match(Result, m_Add(m_Instruction(I), m_Zero())) &&
-      I->getOpcode() == Instruction::Mul)
+      match(Result, m_Instruction(I)) && I->getOpcode() == Instruction::Mul)
     I->setHasNoUnsignedWrap();
 
   // If we had a constant expression GEP on the other side offsetting the
   // pointer, subtract it from the offset we have.
   if (GEP2) {
     Value *Offset = EmitGEPOffset(GEP2);
     Result = Builder.CreateSub(Result, Offset, "gepdiff");
   }