Hi jmolloy, rsmith,
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 [[
https://llvm.org/bugs/show_bug.cgi?id=23517 | 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?
http://reviews.llvm.org/D10045
Files:
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/neon-immediate-ubsan.c
utils/TableGen/NeonEmitter.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: lib/CodeGen/CGBuiltin.cpp
===================================================================
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -3479,6 +3479,13 @@
}
}
+ // 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++) {
@@ -3541,7 +3548,17 @@
continue;
}
}
- Ops.push_back(EmitScalarExpr(E->getArg(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));
+ }
}
switch (BuiltinID) {
@@ -4242,9 +4259,27 @@
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++)
- Ops.push_back(EmitScalarExpr(E->getArg(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(
Index: test/CodeGen/neon-immediate-ubsan.c
===================================================================
--- /dev/null
+++ test/CodeGen/neon-immediate-ubsan.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -triple armv7s-linux-gnu -emit-llvm -O1 -o - %s \
+// RUN: -target-feature +neon -target-cpu cortex-a8 \
+// RUN: -fsanitize=signed-integer-overflow \
+// RUN: | FileCheck %s --check-prefix=CHECK --check-prefix=ARMV7
+
+// RUN: %clang_cc1 -triple aarch64-unknown-unknown -emit-llvm -O1 -o - %s \
+// RUN: -target-feature +neon -target-cpu cortex-a53 \
+// RUN: -fsanitize=signed-integer-overflow \
+// RUN: | FileCheck %s --check-prefix=CHECK --check-prefix=AARCH64
+
+// Verify we emit constants for "immediate" builtin arguments.
+// Emitting a scalar expression can make the immediate be generated as
+// overflow intrinsics, if the overflow sanitizer is enabled.
+
+// PR23517
+
+#include <arm_neon.h>
+
+int32x2_t test_vqrshrn_n_s64(int64x2_t a) {
+ // CHECK-LABEL: @test_vqrshrn_n_s64
+ // CHECK-AARCH64: call <2 x i32> @llvm.aarch64.neon.sqrshrn.v2i32(<2 x i64> {{.*}}, i32 1)
+ // CHECK-ARMV7: call <2 x i32> @llvm.arm.neon.vqrshiftns.v2i32(<2 x i64> {{.*}}, <2 x i64> <i64 -1, i64 -1>)
+ return vqrshrn_n_s64(a, 0 + 1);
+}
Index: utils/TableGen/NeonEmitter.cpp
===================================================================
--- utils/TableGen/NeonEmitter.cpp
+++ utils/TableGen/NeonEmitter.cpp
@@ -131,22 +131,22 @@
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) {}
-
- Type(TypeSpec TS, char CharMod)
- : TS(TS), Float(false), Signed(false), Void(false), Poly(false),
+ : 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), Immediate(false), Void(false),
+ Poly(false), Constant(false), Pointer(false), ScalarForMangling(false),
NoManglingQ(false), Bitwidth(0), ElementBitwidth(0), NumVectors(0) {
applyModifier(CharMod);
}
@@ -167,6 +167,7 @@
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; }
@@ -192,6 +193,14 @@
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() {
@@ -600,6 +609,12 @@
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 += "*";
@@ -853,13 +868,15 @@
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;
@@ -1019,9 +1036,8 @@
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();
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
