[CodeGen][NEON] Emit constants, not ScalarExprs, for "immediate" intrinsic arguments.

[ab]

Summary

On ARM/AArch64, we currently always use EmitScalarExpr for the immediate builtin arguments, instead of directly emitting the constant. When the overflow sanitizer is enabled, this generates overflow intrinsics instead of constants, breaking assumptions in various places (see PR23517).

Instead, use the knowledge of "immediates" to directly emit the constant.

I don't think there's much room for factoring out the operand emission code; if folks feel strongly I can give it another shot!

Note that the NeonEmitter changes can be simplified to the somewhat hacky:

if (hasImmediate() && getImmediateIdx() == I) {
  S += "I";
  T.makeInteger(32, true);
}

in getBuiltinTypeStr. James, which do you prefer?

[t.p.northover]

Hi Ahmed,

This looks reasonable to me, and I see Richard suggested the overall approach.

Tim.

cfe/trunk/lib/CodeGen/CGBuiltin.cpp

if (CRCIntrinsicID != Intrinsic::not_intrinsic) {
     } else {
       Arg1 = Builder.CreateZExtOrBitCast(Arg1, Int32Ty);
 
       Function *F = CGM.getIntrinsic(CRCIntrinsicID);
       return Builder.CreateCall(F, {Arg0, Arg1});
     }
   }
 
+  // Find out if any arguments are required to be integer constant
+  // expressions.
+  unsigned ICEArguments = 0;
+  ASTContext::GetBuiltinTypeError Error;
+  getContext().GetBuiltinType(BuiltinID, Error, &ICEArguments);
+  assert(Error == ASTContext::GE_None && "Should not codegen an error");
+
   SmallVector<Value*, 4> Ops;
   llvm::Value *Align = nullptr;
   for (unsigned i = 0, e = E->getNumArgs() - 1; i != e; i++) {
     if (i == 0) {
       switch (BuiltinID) {
       case NEON::BI__builtin_neon_vld1_v:
       case NEON::BI__builtin_neon_vld1q_v:
       case NEON::BI__builtin_neon_vld1q_lane_v:
if (i == 1) {
         // we'll use it later.
         std::pair<llvm::Value*, unsigned> Src =
             EmitPointerWithAlignment(E->getArg(1));
         Ops.push_back(Src.first);
         Align = Builder.getInt32(Src.second);
         continue;
       }
     }
+
+    if ((ICEArguments & (1 << i)) == 0) {
       Ops.push_back(EmitScalarExpr(E->getArg(i)));
+    } else {
+      // If this is required to be a constant, constant fold it so that we know
+      // that the generated intrinsic gets a ConstantInt.
+      llvm::APSInt Result;
+      bool IsConst = E->getArg(i)->isIntegerConstantExpr(Result, getContext());
+      assert(IsConst && "Constant arg isn't actually constant?"); (void)IsConst;
+      Ops.push_back(llvm::ConstantInt::get(getLLVMContext(), Result));
+    }
   }
 
   switch (BuiltinID) {
   default: break;
   // vget_lane and vset_lane are not overloaded and do not have an extra
   // argument that specifies the vector type.
   case NEON::BI__builtin_neon_vget_lane_i8:
   case NEON::BI__builtin_neon_vget_lane_i16:
if (CRCIntrinsicID != Intrinsic::not_intrinsic) {
     Function *F = CGM.getIntrinsic(CRCIntrinsicID);
 
     llvm::Type *DataTy = F->getFunctionType()->getParamType(1);
     Arg1 = Builder.CreateZExtOrBitCast(Arg1, DataTy);
 
     return Builder.CreateCall(F, {Arg0, Arg1});
   }
 
+  // Find out if any arguments are required to be integer constant
+  // expressions.
+  unsigned ICEArguments = 0;
+  ASTContext::GetBuiltinTypeError Error;
+  getContext().GetBuiltinType(BuiltinID, Error, &ICEArguments);
+  assert(Error == ASTContext::GE_None && "Should not codegen an error");
+
   llvm::SmallVector<Value*, 4> Ops;
-  for (unsigned i = 0, e = E->getNumArgs() - 1; i != e; i++)
+  for (unsigned i = 0, e = E->getNumArgs() - 1; i != e; i++) {
+    if ((ICEArguments & (1 << i)) == 0) {
       Ops.push_back(EmitScalarExpr(E->getArg(i)));
+    } else {
+      // If this is required to be a constant, constant fold it so that we know
+      // that the generated intrinsic gets a ConstantInt.
+      llvm::APSInt Result;
+      bool IsConst = E->getArg(i)->isIntegerConstantExpr(Result, getContext());
+      assert(IsConst && "Constant arg isn't actually constant?");
+      (void)IsConst;
+      Ops.push_back(llvm::ConstantInt::get(getLLVMContext(), Result));
+    }
+  }
 
   auto SISDMap = makeArrayRef(AArch64SISDIntrinsicMap);
   const NeonIntrinsicInfo *Builtin = findNeonIntrinsicInMap(
       SISDMap, BuiltinID, AArch64SISDIntrinsicsProvenSorted);
 
   if (Builtin) {
     Ops.push_back(EmitScalarExpr(E->getArg(E->getNumArgs() - 1)));
     Value *Result = EmitCommonNeonSISDBuiltinExpr(*this, *Builtin, Ops, E);

cfe/trunk/utils/TableGen/NeonEmitter.cpp

 // Type
 //===----------------------------------------------------------------------===//
 
 /// A Type. Not much more to say here.
 class Type {
 private:
   TypeSpec TS;
 
-  bool Float, Signed, Void, Poly, Constant, Pointer;
+  bool Float, Signed, Immediate, Void, Poly, Constant, Pointer;
   // ScalarForMangling and NoManglingQ are really not suited to live here as
   // they are not related to the type. But they live in the TypeSpec (not the
   // prototype), so this is really the only place to store them.
   bool ScalarForMangling, NoManglingQ;
   unsigned Bitwidth, ElementBitwidth, NumVectors;
 
 public:
   Type()
-      : Float(false), Signed(false), Void(true), Poly(false), Constant(false),
-        Pointer(false), ScalarForMangling(false), NoManglingQ(false),
-        Bitwidth(0), ElementBitwidth(0), NumVectors(0) {}
+      : Float(false), Signed(false), Immediate(false), Void(true), Poly(false),
+        Constant(false), Pointer(false), ScalarForMangling(false),
+        NoManglingQ(false), Bitwidth(0), ElementBitwidth(0), NumVectors(0) {}
 
   Type(TypeSpec TS, char CharMod)
-      : TS(TS), Float(false), Signed(false), Void(false), Poly(false),
-        Constant(false), Pointer(false), ScalarForMangling(false),
+      : TS(TS), Float(false), Signed(false), Immediate(false), Void(false),
+        Poly(false), Constant(false), Pointer(false), ScalarForMangling(false),
         NoManglingQ(false), Bitwidth(0), ElementBitwidth(0), NumVectors(0) {
     applyModifier(CharMod);
   }
 
   /// Returns a type representing "void".
   static Type getVoid() { return Type(); }
 
   bool operator==(const Type &Other) const { return str() == Other.str(); }
   bool operator!=(const Type &Other) const { return !operator==(Other); }
 
   //
   // Query functions
   //
   bool isScalarForMangling() const { return ScalarForMangling; }
   bool noManglingQ() const { return NoManglingQ; }
 
   bool isPointer() const { return Pointer; }
   bool isFloating() const { return Float; }
   bool isInteger() const { return !Float && !Poly; }
   bool isSigned() const { return Signed; }
+  bool isImmediate() const { return Immediate; }
   bool isScalar() const { return NumVectors == 0; }
   bool isVector() const { return NumVectors > 0; }
   bool isFloat() const { return Float && ElementBitwidth == 32; }
   bool isDouble() const { return Float && ElementBitwidth == 64; }
   bool isHalf() const { return Float && ElementBitwidth == 16; }
   bool isPoly() const { return Poly; }
   bool isChar() const { return ElementBitwidth == 8; }
   bool isShort() const { return !Float && ElementBitwidth == 16; }
public:
   // Mutator functions
   //
   void makeUnsigned() { Signed = false; }
   void makeSigned() { Signed = true; }
   void makeInteger(unsigned ElemWidth, bool Sign) {
     Float = false;
     Poly = false;
     Signed = Sign;
+    Immediate = false;
+    ElementBitwidth = ElemWidth;
+  }
+  void makeImmediate(unsigned ElemWidth) {
+    Float = false;
+    Poly = false;
+    Signed = true;
+    Immediate = true;
     ElementBitwidth = ElemWidth;
   }
   void makeScalar() {
     Bitwidth = ElementBitwidth;
     NumVectors = 0;
   }
   void makeOneVector() {
     assert(isVector());
else
     }
 
   if (isChar() && !Pointer)
     // Make chars explicitly signed.
     S = "S" + S;
   else if (isInteger() && !Pointer && !Signed)
     S = "U" + S;
 
+  // Constant indices are "int", but have the "constant expression" modifier.
+  if (isImmediate()) {
+    assert(isInteger() && isSigned());
+    S = "I" + S;
+  }
+
   if (isScalar()) {
     if (Constant) S += "C";
     if (Pointer) S += "*";
     return S;
   }
 
   std::string Ret;
   for (unsigned I = 0; I < NumVectors; ++I)
case 'n':
     ElementBitwidth *= 2;
     break;
   case 'i':
     Float = false;
     Poly = false;
     ElementBitwidth = Bitwidth = 32;
     NumVectors = 0;
     Signed = true;
+    Immediate = true;
     break;
   case 'l':
     Float = false;
     Poly = false;
     ElementBitwidth = Bitwidth = 64;
     NumVectors = 0;
     Signed = false;
+    Immediate = true;
     break;
   case 'z':
     ElementBitwidth /= 2;
     Bitwidth = ElementBitwidth;
     NumVectors = 0;
     break;
   case 'r':
     ElementBitwidth *= 2;
if (LocalCK == ClassB && !T.isScalar() && !ForcedFloatingType)
       T.makeInteger(8, true);
     // Halves always get converted to 8-bit elements.
     if (T.isHalf() && T.isVector() && !T.isScalarForMangling())
       T.makeInteger(8, true);
 
     if (LocalCK == ClassI)
       T.makeSigned();
 
-    // Constant indices are always just "int".
     if (hasImmediate() && getImmediateIdx() == I)
-      T.makeInteger(32, true);
+      T.makeImmediate(32);
 
     S += T.builtin_str();
   }
 
   // Extra constant integer to hold type class enum for this function, e.g. s8
   if (LocalCK == ClassB)
     S += "i";