Honor the fwrapv option when generating the inbounds GEP .

[umesh.kalappa0]

Summary

Fix for the issue :https://github.com/llvm/llvm-project/issues/59580

Where we don't emit the inbounds flag for the GEP when wrapv is enabled .

[nikic]

CreateConstArrayGEP currently always creates an inbounds GEP. You need to modify it make inbounds optional or use a different method, not suppress it in the constant folding infrastructure.

[nikic]

You also shouldn't need to introduce any new handling for the fwrapv option. I'm not sure how exactly it is currently threaded through, but there must be existing handling for it already. For example, there is isSignedOverflowDefined() on LangOptions.

[umesh.kalappa0]

In D142872#4090317, @nikic wrote:

You also shouldn't need to introduce any new handling for the fwrapv option. I'm not sure how exactly it is currently threaded through, but there must be existing handling for it already. For example, there is isSignedOverflowDefined() on LangOptions.

Make sense here .

[umesh.kalappa0]

In D142872#4090304, @nikic wrote:

CreateConstArrayGEP currently always creates an inbounds GEP. You need to modify it make inbounds optional or use a different method, not suppress it in the constant folding infrastructure.

Thank you very much for the feedback and

We tried calling other overload like "CreateConstGEP" ,but that didn't help due to below call from "ConstantFoldGetElementPtr" ,that was recursive call from "ConstantExpr::getGetElementPtr" .

return ConstantExpr::getGetElementPtr(PointeeTy, C, Idxs, /*InBounds=*/true, InRangeIndex);

So we thought of disabling the call like "ConstantExpr::getGetElementPtr" with InBounds=true for wrapv and changes has its consensus so we end up changes that is shared for review.

[nikic]

@umesh.kalappa0 I see. That part is actually a bug in LLVM. I'm working on a fix.

[nikic]

@umesh.kalappa0 This issue should be fixed by https://github.com/llvm/llvm-project/commit/5f01a626dd0615df49773d419c75aeb33666ee83. Can you please give it another try?

[umesh.kalappa0]

In D142872#4093093, @nikic wrote:

@umesh.kalappa0 This issue should be fixed by https://github.com/llvm/llvm-project/commit/5f01a626dd0615df49773d419c75aeb33666ee83. Can you please give it another try?

Thank you @nikic for the quick changes ,and updated the review

[umesh.kalappa0]

@nikic ,is that changes are ok ?

[nikic]

I've found another bug in LLVM, fixed with https://github.com/llvm/llvm-project/commit/78f88082de3627ea04501c83a08f52cf1e60b4f7. After that change, the test case from https://github.com/llvm/llvm-project/issues/59580 appears to be handled correctly, it contains this in the final IR:

br i1 icmp ugt (ptr getelementptr ([0 x i8], ptr @end, i64 0, i64 536870911), ptr @end), label %if.then, label %if.else9

As such, I'm not sure if we actually need any change in Clang, as it seems like this was ultimately due to LLVM bugs. At least we would need a different test case, as the inbounds.c test case is already getting compiled correctly without this patch.

llvm/include/llvm/IR/ConstantFold.h

29

These changes shouldn't be needed anymore.

[nikic]

The original issue has been fixed by the mentioned LLVM changes. As such, I don't think we need to actually do anything on the Clang side. Please let me know if you're still seeing issues in some case...

clang/lib/CodeGen/CGExpr.cpp

Address CodeGenFunction::EmitArrayToPointerDecay(const Expr *E,
   llvm::Type *NewTy = ConvertType(E->getType());
   Addr = Builder.CreateElementBitCast(Addr, NewTy);
 
   // Note that VLA pointers are always decayed, so we don't need to do
   // anything here.
   if (!E->getType()->isVariableArrayType()) {
     assert(isa<llvm::ArrayType>(Addr.getElementType()) &&
            "Expected pointer to array");
+
+    if (getLangOpts().isSignedOverflowDefined()) {
+      llvm::setSignedWrap(true);
+      Addr = Builder.CreateConstGEP(Addr, 0, "arraydecay");
+    }
+    else {
       Addr = Builder.CreateConstArrayGEP(Addr, 0, "arraydecay");
     }
 
+  }
+
   // The result of this decay conversion points to an array element within the
   // base lvalue. However, since TBAA currently does not support representing
   // accesses to elements of member arrays, we conservatively represent accesses
   // to the pointee object as if it had no any base lvalue specified.
   // TODO: Support TBAA for member arrays.
   QualType EltType = E->getType()->castAsArrayTypeUnsafe()->getElementType();
   if (BaseInfo) *BaseInfo = LV.getBaseInfo();
   if (TBAAInfo) *TBAAInfo = CGM.getTBAAAccessInfo(EltType);

clang/test/CodeGen/inbounds.c

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fwrapv -emit-llvm -o - %s | FileCheck %s 
+
+extern char base[];
+
+char *test() {
+    return base + 1;
+}
+
+// CHECK: ret ptr getelementptr   (i8, ptr @base, i64 1)

llvm/include/llvm/IR/ConstantFold.h

 #ifndef LLVM_IR_CONSTANTFOLD_H
 #define LLVM_IR_CONSTANTFOLD_H
 
 #include "llvm/IR/InstrTypes.h"
 #include <optional>
 
 namespace llvm {
   template <typename T> class ArrayRef;
+  inline bool SignedWrap = false;
+
   class Value;
   class Constant;
   class Type;
 
+  bool getSignedWrap(void);
+  void setSignedWrap(bool Wrap);
+
   // Constant fold various types of instruction...
   Constant *ConstantFoldCastInstruction(
     unsigned opcode,     ///< The opcode of the cast
     Constant *V,         ///< The source constant
     Type *DestTy   ///< The destination type
   );
   Constant *ConstantFoldSelectInstruction(Constant *Cond,
                                           Constant *V1, Constant *V2);

llvm/lib/IR/ConstantFold.cpp

case Instruction::ZExt: {
     }
 
     // TODO: Handle the 'partially zero' case.
     return nullptr;
   }
   }
 }
 
+bool llvm::getSignedWrap() {
+  return llvm::SignedWrap;
+}
+
+void llvm::setSignedWrap(bool sWrap) {
+  llvm::SignedWrap=sWrap;
+}
+
 Constant *llvm::ConstantFoldCastInstruction(unsigned opc, Constant *V,
                                             Type *DestTy) {
   if (isa<PoisonValue>(V))
     return PoisonValue::get(DestTy);
 
   if (isa<UndefValue>(V)) {
     // zext(undef) = 0, because the top bits will be zero.
     // sext(undef) = 0, because the top bits will all be the same.