target ABI: improve call parameters extensions handling #100757
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-backend-x86 Author: Jonas Paulsson (JonPsson1) ChangesAfter a long delay, I am coming back to this patch due to recent interest. After rebasing, it seems that the old basic idea still works: introduce a NoExt attribute that can be added on narrow integers when neither sign- or zeroextension is required (typically "struct in reg"). Use that to detect cases of missing extension attributes. This was last worked on (and begun) at https://reviews.llvm.org/D112942 where I took off from it to work on fixing this in some places that where then found to be broken (libcalls, sanitizers). There was also some discussion on #54530. In summary: Common-code:
On SystemZ:
At this point, the above seem to work and I have used clang to build SPEC and also LLVM itself without any cases of missing NoExt reported. So on SystemZ, this patch can already be applied to use the check of arguments (disabled with -no-argext-abi-check => patch NFC). This has however generally been ignored in testing so ~400 (only a few of them fixed so far) tests have missing extension attributes which need to be fixed before this can be committed. I have built with -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;llvm;mlir" -DLLVM_ENABLE_RUNTIMES="openmp;compiler-rt", and run ninja; ninja check-all to get an overview of what would need to be fixed (I hope those are all the relevant subprojects..?)
Perhaps this could be committed with the check turned off by default, and when all tests eventually have been updated (and any actual problems remaining have been fixed), it could be turned on by default..? Patch is 43.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100757.diff 36 Files Affected:
diff --git a/clang/include/clang/CodeGen/CGFunctionInfo.h b/clang/include/clang/CodeGen/CGFunctionInfo.h
index 811f33407368c..6a163ef592eda 100644
--- a/clang/include/clang/CodeGen/CGFunctionInfo.h
+++ b/clang/include/clang/CodeGen/CGFunctionInfo.h
@@ -116,6 +116,7 @@ class ABIArgInfo {
bool InReg : 1; // isDirect() || isExtend() || isIndirect()
bool CanBeFlattened: 1; // isDirect()
bool SignExt : 1; // isExtend()
+ bool ZeroExt : 1; // isExtend()
bool canHavePaddingType() const {
return isDirect() || isExtend() || isIndirect() || isIndirectAliased() ||
@@ -137,7 +138,7 @@ class ABIArgInfo {
PaddingInReg(false), InAllocaSRet(false),
InAllocaIndirect(false), IndirectByVal(false), IndirectRealign(false),
SRetAfterThis(false), InReg(false), CanBeFlattened(false),
- SignExt(false) {}
+ SignExt(false), ZeroExt(false) {}
static ABIArgInfo getDirect(llvm::Type *T = nullptr, unsigned Offset = 0,
llvm::Type *Padding = nullptr,
@@ -174,12 +175,12 @@ class ABIArgInfo {
AI.setPaddingType(nullptr);
AI.setDirectOffset(0);
AI.setDirectAlign(0);
- AI.setSignExt(false);
+ AI.setZeroExt(true);
return AI;
}
// ABIArgInfo will record the argument as being extended based on the sign
- // of its type.
+ // of its type. Produces a sign or zero extension.
static ABIArgInfo getExtend(QualType Ty, llvm::Type *T = nullptr) {
assert(Ty->isIntegralOrEnumerationType() && "Unexpected QualType");
if (Ty->hasSignedIntegerRepresentation())
@@ -187,6 +188,13 @@ class ABIArgInfo {
return getZeroExtend(Ty, T);
}
+ // Struct in register marked explicitly as not needing extension.
+ static ABIArgInfo getNoExtend(llvm::IntegerType *T) {
+ auto AI = ABIArgInfo(Extend);
+ AI.setCoerceToType(T);
+ return AI;
+ }
+
static ABIArgInfo getExtendInReg(QualType Ty, llvm::Type *T = nullptr) {
auto AI = getExtend(Ty, T);
AI.setInReg(true);
@@ -326,7 +334,7 @@ class ABIArgInfo {
}
bool isSignExt() const {
- assert(isExtend() && "Invalid kind!");
+ assert(isExtend() && (SignExt + ZeroExt <= 1) && "Invalid kind / flags!");
return SignExt;
}
void setSignExt(bool SExt) {
@@ -334,6 +342,20 @@ class ABIArgInfo {
SignExt = SExt;
}
+ bool isZeroExt() const {
+ assert(isExtend() && (SignExt + ZeroExt <= 1) && "Invalid kind / flags!");
+ return ZeroExt;
+ }
+ void setZeroExt(bool ZExt) {
+ assert(isExtend() && "Invalid kind!");
+ ZeroExt = ZExt;
+ }
+
+ bool isNoExt() const {
+ assert(isExtend() && (SignExt + ZeroExt <= 1) && "Invalid kind / flags!");
+ return !SignExt && !ZeroExt;
+ }
+
llvm::Type *getPaddingType() const {
return (canHavePaddingType() ? PaddingType : nullptr);
}
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 234a9c16e39df..1150657df2506 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2205,7 +2205,7 @@ static bool DetermineNoUndef(QualType QTy, CodeGenTypes &Types,
if (AI.getKind() == ABIArgInfo::Indirect ||
AI.getKind() == ABIArgInfo::IndirectAliased)
return true;
- if (AI.getKind() == ABIArgInfo::Extend)
+ if (AI.getKind() == ABIArgInfo::Extend && !AI.isNoExt())
return true;
if (!DL.typeSizeEqualsStoreSize(Ty))
// TODO: This will result in a modest amount of values not marked noundef
@@ -2585,9 +2585,12 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
case ABIArgInfo::Extend:
if (RetAI.isSignExt())
RetAttrs.addAttribute(llvm::Attribute::SExt);
- else
+ else if (RetAI.isZeroExt())
RetAttrs.addAttribute(llvm::Attribute::ZExt);
- [[fallthrough]];
+ else
+ RetAttrs.addAttribute(llvm::Attribute::NoExt);
+ [[fallthrough]];
+
case ABIArgInfo::Direct:
if (RetAI.getInReg())
RetAttrs.addAttribute(llvm::Attribute::InReg);
@@ -2726,9 +2729,12 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
case ABIArgInfo::Extend:
if (AI.isSignExt())
Attrs.addAttribute(llvm::Attribute::SExt);
- else
+ else if (AI.isZeroExt())
Attrs.addAttribute(llvm::Attribute::ZExt);
+ else
+ Attrs.addAttribute(llvm::Attribute::NoExt);
[[fallthrough]];
+
case ABIArgInfo::Direct:
if (ArgNo == 0 && FI.isChainCall())
Attrs.addAttribute(llvm::Attribute::Nest);
diff --git a/clang/lib/CodeGen/Targets/SystemZ.cpp b/clang/lib/CodeGen/Targets/SystemZ.cpp
index 4d61f51379346..bb7582ffc70b1 100644
--- a/clang/lib/CodeGen/Targets/SystemZ.cpp
+++ b/clang/lib/CodeGen/Targets/SystemZ.cpp
@@ -445,16 +445,20 @@ ABIArgInfo SystemZABIInfo::classifyArgumentType(QualType Ty) const {
return getNaturalAlignIndirect(Ty, /*ByVal=*/false);
// The structure is passed as an unextended integer, a float, or a double.
- llvm::Type *PassTy;
if (isFPArgumentType(SingleElementTy)) {
assert(Size == 32 || Size == 64);
+ llvm::Type *PassTy;
if (Size == 32)
PassTy = llvm::Type::getFloatTy(getVMContext());
else
PassTy = llvm::Type::getDoubleTy(getVMContext());
- } else
- PassTy = llvm::IntegerType::get(getVMContext(), Size);
- return ABIArgInfo::getDirect(PassTy);
+ return ABIArgInfo::getDirect(PassTy);
+ } else {
+ llvm::IntegerType *PassTy = llvm::IntegerType::get(getVMContext(), Size);
+ if (Size <= 32)
+ return ABIArgInfo::getNoExtend(PassTy);
+ return ABIArgInfo::getDirect(PassTy);
+ }
}
// Non-structure compounds are passed indirectly.
diff --git a/clang/test/CodeGen/SystemZ/systemz-abi-vector.c b/clang/test/CodeGen/SystemZ/systemz-abi-vector.c
index 6a06d15f22aa3..221aa2f4a2f46 100644
--- a/clang/test/CodeGen/SystemZ/systemz-abi-vector.c
+++ b/clang/test/CodeGen/SystemZ/systemz-abi-vector.c
@@ -146,17 +146,17 @@ v1f128 pass_v1f128(v1f128 arg) { return arg; }
struct agg_v1i8 { v1i8 a; };
struct agg_v1i8 pass_agg_v1i8(struct agg_v1i8 arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_v1i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v1i8) align 1 %{{.*}}, i8 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_v1i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v1i8) align 1 %{{.*}}, i8 noext %{{.*}})
// CHECK-VECTOR-LABEL: define{{.*}} void @pass_agg_v1i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v1i8) align 1 %{{.*}}, <1 x i8> %{{.*}})
struct agg_v2i8 { v2i8 a; };
struct agg_v2i8 pass_agg_v2i8(struct agg_v2i8 arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_v2i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v2i8) align 2 %{{.*}}, i16 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_v2i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v2i8) align 2 %{{.*}}, i16 noext %{{.*}})
// CHECK-VECTOR-LABEL: define{{.*}} void @pass_agg_v2i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v2i8) align 2 %{{.*}}, <2 x i8> %{{.*}})
struct agg_v4i8 { v4i8 a; };
struct agg_v4i8 pass_agg_v4i8(struct agg_v4i8 arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_v4i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v4i8) align 4 %{{.*}}, i32 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_v4i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v4i8) align 4 %{{.*}}, i32 noext %{{.*}})
// CHECK-VECTOR-LABEL: define{{.*}} void @pass_agg_v4i8(ptr dead_on_unwind noalias writable sret(%struct.agg_v4i8) align 4 %{{.*}}, <4 x i8> %{{.*}})
struct agg_v8i8 { v8i8 a; };
@@ -189,8 +189,8 @@ struct agg_novector2 pass_agg_novector2(struct agg_novector2 arg) { return arg;
struct agg_novector3 { v4i8 a; int : 0; };
struct agg_novector3 pass_agg_novector3(struct agg_novector3 arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_novector3(ptr dead_on_unwind noalias writable sret(%struct.agg_novector3) align 4 %{{.*}}, i32 %{{.*}})
-// CHECK-VECTOR-LABEL: define{{.*}} void @pass_agg_novector3(ptr dead_on_unwind noalias writable sret(%struct.agg_novector3) align 4 %{{.*}}, i32 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_novector3(ptr dead_on_unwind noalias writable sret(%struct.agg_novector3) align 4 %{{.*}}, i32 noext %{{.*}})
+// CHECK-VECTOR-LABEL: define{{.*}} void @pass_agg_novector3(ptr dead_on_unwind noalias writable sret(%struct.agg_novector3) align 4 %{{.*}}, i32 noext %{{.*}})
struct agg_novector4 { v4i8 a __attribute__((aligned (8))); };
struct agg_novector4 pass_agg_novector4(struct agg_novector4 arg) { return arg; }
diff --git a/clang/test/CodeGen/SystemZ/systemz-abi.c b/clang/test/CodeGen/SystemZ/systemz-abi.c
index 3e39425e57f17..997c685a89850 100644
--- a/clang/test/CodeGen/SystemZ/systemz-abi.c
+++ b/clang/test/CodeGen/SystemZ/systemz-abi.c
@@ -86,11 +86,11 @@ _Complex long double pass_complex_longdouble(_Complex long double arg) { return
struct agg_1byte { char a[1]; };
struct agg_1byte pass_agg_1byte(struct agg_1byte arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_1byte(ptr dead_on_unwind noalias writable sret(%struct.agg_1byte) align 1 %{{.*}}, i8 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_1byte(ptr dead_on_unwind noalias writable sret(%struct.agg_1byte) align 1 %{{.*}}, i8 noext %{{.*}})
struct agg_2byte { char a[2]; };
struct agg_2byte pass_agg_2byte(struct agg_2byte arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_2byte(ptr dead_on_unwind noalias writable sret(%struct.agg_2byte) align 1 %{{.*}}, i16 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_2byte(ptr dead_on_unwind noalias writable sret(%struct.agg_2byte) align 1 %{{.*}}, i16 noext %{{.*}})
struct agg_3byte { char a[3]; };
struct agg_3byte pass_agg_3byte(struct agg_3byte arg) { return arg; }
@@ -98,7 +98,7 @@ struct agg_3byte pass_agg_3byte(struct agg_3byte arg) { return arg; }
struct agg_4byte { char a[4]; };
struct agg_4byte pass_agg_4byte(struct agg_4byte arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_4byte(ptr dead_on_unwind noalias writable sret(%struct.agg_4byte) align 1 %{{.*}}, i32 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_4byte(ptr dead_on_unwind noalias writable sret(%struct.agg_4byte) align 1 %{{.*}}, i32 noext %{{.*}})
struct agg_5byte { char a[5]; };
struct agg_5byte pass_agg_5byte(struct agg_5byte arg) { return arg; }
@@ -126,7 +126,7 @@ struct agg_16byte pass_agg_16byte(struct agg_16byte arg) { return arg; }
struct agg_float { float a; };
struct agg_float pass_agg_float(struct agg_float arg) { return arg; }
// HARD-FLOAT-LABEL: define{{.*}} void @pass_agg_float(ptr dead_on_unwind noalias writable sret(%struct.agg_float) align 4 %{{.*}}, float %{{.*}})
-// SOFT-FLOAT-LABEL: define{{.*}} void @pass_agg_float(ptr dead_on_unwind noalias writable sret(%struct.agg_float) align 4 %{{.*}}, i32 %{{.*}})
+// SOFT-FLOAT-LABEL: define{{.*}} void @pass_agg_float(ptr dead_on_unwind noalias writable sret(%struct.agg_float) align 4 %{{.*}}, i32 noext %{{.*}})
struct agg_double { double a; };
struct agg_double pass_agg_double(struct agg_double arg) { return arg; }
@@ -159,14 +159,14 @@ struct agg_nofloat2 pass_agg_nofloat2(struct agg_nofloat2 arg) { return arg; }
struct agg_nofloat3 { float a; int : 0; };
struct agg_nofloat3 pass_agg_nofloat3(struct agg_nofloat3 arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_agg_nofloat3(ptr dead_on_unwind noalias writable sret(%struct.agg_nofloat3) align 4 %{{.*}}, i32 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_agg_nofloat3(ptr dead_on_unwind noalias writable sret(%struct.agg_nofloat3) align 4 %{{.*}}, i32 noext %{{.*}})
// Union types likewise are *not* float-like aggregate types
union union_float { float a; };
union union_float pass_union_float(union union_float arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @pass_union_float(ptr dead_on_unwind noalias writable sret(%union.union_float) align 4 %{{.*}}, i32 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @pass_union_float(ptr dead_on_unwind noalias writable sret(%union.union_float) align 4 %{{.*}}, i32 noext %{{.*}})
union union_double { double a; };
union union_double pass_union_double(union union_double arg) { return arg; }
diff --git a/clang/test/CodeGen/SystemZ/systemz-abi.cpp b/clang/test/CodeGen/SystemZ/systemz-abi.cpp
index 06be85421ba17..b13aedfca464b 100644
--- a/clang/test/CodeGen/SystemZ/systemz-abi.cpp
+++ b/clang/test/CodeGen/SystemZ/systemz-abi.cpp
@@ -7,7 +7,7 @@
class agg_float_class { float a; };
class agg_float_class pass_agg_float_class(class agg_float_class arg) { return arg; }
// CHECK-LABEL: define{{.*}} void @_Z20pass_agg_float_class15agg_float_class(ptr dead_on_unwind noalias writable sret(%class.agg_float_class) align 4 %{{.*}}, float %{{.*}})
-// SOFT-FLOAT-LABEL: define{{.*}} void @_Z20pass_agg_float_class15agg_float_class(ptr dead_on_unwind noalias writable sret(%class.agg_float_class) align 4 %{{.*}}, i32 %{{.*}})
+// SOFT-FLOAT-LABEL: define{{.*}} void @_Z20pass_agg_float_class15agg_float_class(ptr dead_on_unwind noalias writable sret(%class.agg_float_class) align 4 %{{.*}}, i32 noext %{{.*}})
class agg_double_class { double a; };
class agg_double_class pass_agg_double_class(class agg_double_class arg) { return arg; }
@@ -18,8 +18,8 @@ class agg_double_class pass_agg_double_class(class agg_double_class arg) { retur
// This structure is passed in a GPR in C++ (and C, checked in systemz-abi.c).
struct agg_float_cpp { float a; int : 0; };
struct agg_float_cpp pass_agg_float_cpp(struct agg_float_cpp arg) { return arg; }
-// CHECK-LABEL: define{{.*}} void @_Z18pass_agg_float_cpp13agg_float_cpp(ptr dead_on_unwind noalias writable sret(%struct.agg_float_cpp) align 4 %{{.*}}, i32 %{{.*}})
-// SOFT-FLOAT-LABEL: define{{.*}} void @_Z18pass_agg_float_cpp13agg_float_cpp(ptr dead_on_unwind noalias writable sret(%struct.agg_float_cpp) align 4 %{{.*}}, i32 %{{.*}})
+// CHECK-LABEL: define{{.*}} void @_Z18pass_agg_float_cpp13agg_float_cpp(ptr dead_on_unwind noalias writable sret(%struct.agg_float_cpp) align 4 %{{.*}}, i32 noext %{{.*}})
+// SOFT-FLOAT-LABEL: define{{.*}} void @_Z18pass_agg_float_cpp13agg_float_cpp(ptr dead_on_unwind noalias writable sret(%struct.agg_float_cpp) align 4 %{{.*}}, i32 noext %{{.*}})
// A field member of empty class type in C++ makes the record nonhomogeneous,
@@ -32,7 +32,7 @@ struct agg_nofloat_empty pass_agg_nofloat_empty(struct agg_nofloat_empty arg) {
struct agg_float_empty { float a; [[no_unique_address]] empty dummy; };
struct agg_float_empty pass_agg_float_empty(struct agg_float_empty arg) { return arg; }
// CHECK-LABEL: define{{.*}} void @_Z20pass_agg_float_empty15agg_float_empty(ptr dead_on_unwind noalias writable sret(%struct.agg_float_empty) align 4 %{{.*}}, float %{{.*}})
-// SOFT-FLOAT-LABEL: define{{.*}} void @_Z20pass_agg_float_empty15agg_float_empty(ptr dead_on_unwind noalias writable sret(%struct.agg_float_empty) align 4 %{{.*}}, i32 %{{.*}})
+// SOFT-FLOAT-LABEL: define{{.*}} void @_Z20pass_agg_float_empty15agg_float_empty(ptr dead_on_unwind noalias writable sret(%struct.agg_float_empty) align 4 %{{.*}}, i32 noext %{{.*}})
struct agg_nofloat_emptyarray { float a; [[no_unique_address]] empty dummy[3]; };
struct agg_nofloat_emptyarray pass_agg_nofloat_emptyarray(struct agg_nofloat_emptyarray arg) { return arg; }
// CHECK-LABEL: define{{.*}} void @_Z27pass_agg_nofloat_emptyarray22agg_nofloat_emptyarray(ptr dead_on_unwind noalias writable sret(%struct.agg_nofloat_emptyarray) align 4 %{{.*}}, i64 %{{.*}})
@@ -48,7 +48,7 @@ struct emptybase { [[no_unique_address]] empty dummy; };
struct agg_float_emptybase : emptybase { float a; };
struct agg_float_emptybase pass_agg_float_emptybase(struct agg_float_emptybase arg) { return arg; }
// CHECK-LABEL: define{{.*}} void @_Z24pass_agg_float_emptybase19agg_float_emptybase(ptr dead_on_unwind noalias writable sret(%struct.agg_float_emptybase) align 4 %{{.*}}, float %{{.*}})
-// SOFT-FLOAT-LABEL: define{{.*}} void @_Z24pass_agg_float_emptybase19agg_float_emptybase(ptr dead_on_unwind noalias writable sret(%struct.agg_float_emptybase) align 4 %{{.*}}, i32 %{{.*}})
+// SOFT-FLOAT-LABEL: define{{.*}} void @_Z24pass_agg_float_emptybase19agg_float_emptybase(ptr dead_on_unwind noalias writable sret(%struct.agg_float_emptybase) align 4 %{{.*}}, i32 noext %{{.*}})
struct noemptybasearray { [[no_unique_address]] empty dummy[3]; };
struct agg_nofloat_emptybasearray : noemptybasearray { float a; };
struct agg_nofloat_emptybasearray pass_agg_nofloat_emptybasearray(struct agg_nofloat_emptybasearray arg) { return arg; }
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index cd86156ec816f..3f60811b878d1 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -1174,6 +1174,10 @@ For example:
Note that any attributes for the function result (``nonnull``,
``signext``) come before the result type.
+If an integer argument to a function is not marked signext/zeroext/noext, the
+kind of extension used is target-specific. Some targets depend for
+correctness on the kind of extension to be explicitly specified.
+
Currently, only the following parameter attributes are defined:
``zeroext``
@@ -1185,6 +1189,9 @@ Currently, only the following parameter attributes are defined:
value should be sign-extended to the extent required by the target's
ABI (which is usually 32-bits) by the caller (for a parameter) or
the callee (for a return value).
+``noext`` This indicates to the code generator that the parameter or return
+ value has the high bits undefined, as for a struct in register, and
+ therefore does not need to be sign or zero extended.
``inreg``
This indicates that this parameter or return value should be treated
in a special target-dependent fashion while emitting code for
@@ -9082,8 +9089,8 @@ This instruction requires several arguments:
convention <callingconv>` the call should use. If none is
specified, the call defaults to using C calling conventions.
#. The optional :ref:`Parameter Attributes <paramattrs>` list for return
- values. Only '``zeroext``', '``signext``', and '``inreg``' attributes
- are valid here.
+ values. Only '``zeroext``', '``signext``', '``noext``', and '``inreg``'
+ attributes are valid here.
#. The optional addrspace attribute can be used to indicate the address space
of the called function. If it is not specified, the program address space
from the :ref:`datalayout string<langref_datalayout>` will be used.
@@ -9178,8 +9185,8 @@ This instruction requires several arguments:
convention <callingconv>` the call should use. If none is
specified, the call defaults to using C calling conventions.
#. The optional :ref:`Parameter Attributes <paramattrs>` list for return
- values. Only '``zeroext``', '``signext``', and '``inreg``' attributes
- are valid here.
+ values. Only '``zeroext``', '``signext``', '``noext``', and '``inreg``'
+ attributes are valid here.
#. The optional addrspace attribute can be used to indicate the address space
of the called function. If it is not specified, the program address space
from the :ref:`datalayout string<langref_datalayout>` will be used.
@@ -12664,8 +12671,8 @@ This instruction requires several arguments:
calling convention of the call must match the calling convention of
the target function, or else the behavior is undefined.
#. The optional :ref:`Parameter Attributes <paramattrs>` list for return
- values. Only '``zeroext``', '``signext``', and '``inreg``' attributes
- are valid here.
+ values. Only '``zeroext``', '``signext``', '``noext``', and '``inreg``'
+ attributes are valid here.
#. The optional addrspace attribute can be used to indicate the address space
of the called function. If it is not specified, the program address space
from the :ref:`datalayout string<langref_datalayout>` will be used.
diff --git a/llvm/include/llvm/Bitcode/LLVMBitCodes.h b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
index fb88f2fe75adb..e690208d1b89b 100644
--- a/llvm/include/llvm/Bitcode/LLVMBitCodes.h
+++ b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
@@ -758,6 +758,7 @@ enum AttributeKindCodes {
ATTR_KIND_SANITIZE_NUMERICAL_STABILITY = 93,
ATTR_KIND_INITIALIZES = 94,
ATTR_KIND_HYBRID_PATCHABLE = 95,
+ ATTR_KIND_NO_EXT = 96,
};
enum ComdatSelectio...
[truncated]
|
JonPsson1
removed
backend:X86
llvm:SelectionDAG
You can test this locally with the following command:git-clang-format --diff f3250858780b37a188875c87e57133f1192b2e60 a74fb7cac005078aef7dc84c9ae753c1e0ff2fb3 --extensions h,c,cpp -- clang/include/clang/CodeGen/CGFunctionInfo.h clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/Targets/SystemZ.cpp clang/test/CodeGen/SystemZ/systemz-abi-vector.c clang/test/CodeGen/SystemZ/systemz-abi.c clang/test/CodeGen/SystemZ/systemz-abi.cpp llvm/include/llvm/Bitcode/LLVMBitCodes.h llvm/include/llvm/CodeGen/TargetCallingConv.h llvm/include/llvm/CodeGen/TargetLowering.h llvm/include/llvm/Target/TargetOptions.h llvm/lib/Bitcode/Reader/BitcodeReader.cpp llvm/lib/Bitcode/Writer/BitcodeWriter.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp llvm/lib/Target/SystemZ/SystemZISelLowering.cpp llvm/lib/Target/SystemZ/SystemZISelLowering.h llvm/lib/Transforms/Utils/CodeExtractor.cpp llvm/tools/llc/llc.cppView the diff from clang-format here.diff --git a/clang/include/clang/CodeGen/CGFunctionInfo.h b/clang/include/clang/CodeGen/CGFunctionInfo.h
index d19f84d198..503e638fce 100644
--- a/clang/include/clang/CodeGen/CGFunctionInfo.h
+++ b/clang/include/clang/CodeGen/CGFunctionInfo.h
@@ -135,10 +135,9 @@ private:
public:
ABIArgInfo(Kind K = Direct)
: TypeData(nullptr), PaddingType(nullptr), DirectAttr{0, 0}, TheKind(K),
- PaddingInReg(false), InAllocaSRet(false),
- InAllocaIndirect(false), IndirectByVal(false), IndirectRealign(false),
- SRetAfterThis(false), InReg(false), CanBeFlattened(false),
- SignExt(false), ZeroExt(false) {}
+ PaddingInReg(false), InAllocaSRet(false), InAllocaIndirect(false),
+ IndirectByVal(false), IndirectRealign(false), SRetAfterThis(false),
+ InReg(false), CanBeFlattened(false), SignExt(false), ZeroExt(false) {}
static ABIArgInfo getDirect(llvm::Type *T = nullptr, unsigned Offset = 0,
llvm::Type *Padding = nullptr,
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index a24ed89e2a..f1369f23d0 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -9835,9 +9835,8 @@ bool SystemZTargetLowering::isFullyInternal(const Function *Fn) const {
}
// Verify that narrow integer arguments are extended as required by the ABI.
-void SystemZTargetLowering::
-verifyNarrowIntegerArgs(const SmallVectorImpl<ISD::OutputArg> &Outs,
- bool IsInternal) const {
+void SystemZTargetLowering::verifyNarrowIntegerArgs(
+ const SmallVectorImpl<ISD::OutputArg> &Outs, bool IsInternal) const {
if (IsInternal || !Subtarget.isTargetELF())
return;
|
llvm/docs/LangRef.rst
Outdated
| ``noext`` This indicates to the code generator that the parameter or return | ||
| value has the high bits undefined, as for a struct in register, and | ||
| therefore does not need to be sign or zero extended. |
There was a problem hiding this comment.
I don't understand how this is different from the default behavior already
There was a problem hiding this comment.
It's not.
The issue is that it's easy to write LLVM code that accidentally skips ABI computations. "noext" is an explicit marker indicating that the code generating the IR does understand the ABI, and decided no extension is necessary. That lets the backend detect whether some transform skipped doing ABI computations.
There was a problem hiding this comment.
Can we do IR verifier for ABI illegal cases instead?
There was a problem hiding this comment.
I don't understand how this improves the situation, or how the IR can "understand the ABI". The IR has its own ABI. This may or may not match the C ABI, the IR ABI is not really defined in terms of C
There was a problem hiding this comment.
There doesn't exist an "IR ABI". If you don't specify a calling convention in IR, it must be C calling convention.
Or you want to call representations out of C ABI definition "IR ABI"? I cannot agree with either. A typical ABI has two merits: well documented and stable. Neither do they have. They are almost exceptions unconsciously generated by front end or written by hand. They exist just because we lack a verifier for them.
My suggestion is to introduce ABI verification code for each target in the IR verifier like this. We may start from cases we care most and gradually check all rules followed by clang front end.
There was a problem hiding this comment.
Can we do IR verifier for ABI illegal cases instead?
In previous discussions it has been agreed to generally let the target decide on how to act on the argument extensions attributes. So we decided to keep the check for these in the (SystemZ) backend. Per my understanding, this is related to the target platform ABI - some targets like SystemZ has specific extension requirements of arguments. Others do not and can therefore ignore these attributes. Therefore, having a general IR verifier for this doesn't quite make sense - unless one would add a new hook as well to check with the target that it depends on valid argument extension attributes.
My suggestion is to introduce ABI verification code for each target in the IR verifier...
I agree this would make sense, but think this could be a follow-up patch once we have the needed parts this patch provides.
The goal is to get some sort of error from code like that, instead of an obscure miscompile.
Yes, if the front-end or an instrumentation pass forgets to add the extension, it could lead to wrong-code that is hard to track down. This is especially treacherous since somebody might work on a target that do not care about this at all and introduce a problem on another target that depends on it. In such a case it would be great if e.g. the SystemZ buildbot would fail by detecting the case with no extension attribute.
There was a problem hiding this comment.
There doesn't exist an "IR ABI".
There absolutely is. A given IR signature has a backend interpretation. That is the IR ABI. It doesn't matter if this is documented or standardized in any way, it is what it is.
My suggestion is to introduce ABI verification code
LLVM is not a clang / C backend
There was a problem hiding this comment.
- That's incorrect. Some implementation defined behavior are subject to change, hence cannot be called an ABI. It just like we cannot assume UB code get the same result between different compilers or from version to version.
- If other front end doesn't specific a calling convention, then it uses C calling convention. See my previous link.
BTW, I'm not opposite to this patch.
There was a problem hiding this comment.
- That's incorrect. Some implementation defined behavior are subject to change, hence cannot be called an ABI. It just like we cannot assume UB code get the same result between different compilers or from version to version.
- If other front end doesn't specific a calling convention, then it uses C calling convention. See my previous link.
Sorry, I don't follow - what part is incorrect according to you? The target ABI is independent of the implementation - it is how things are done on a specific platform. On some machines this type of extensions must be done, and this will not change. Other machines do not care about this at all. How could this become more clear in the LangRef.rst do you mean?
There was a problem hiding this comment.
Sorry, I don't follow - what part is incorrect according to you?
Oh, I should have quoted what I replied for
There doesn't exist an "IR ABI".
There absolutely is. A given IR signature has a backend interpretation. That is the IR ABI. It doesn't matter if this is documented or standardized in any way, it is what it is.
For the question
The target ABI is independent of the implementation - it is how things are done on a specific platform. On some machines this type of extensions must be done, and this will not change. Other machines do not care about this at all. How could this become more clear in the LangRef.rst do you mean?
The target ABI (from C's perspective) is a subset of what IR can represent. I mean to those representation out of the scope. They are subject to change, because ABI can be defined rather late than the implementation and possible different with it. An example is the half type on X86. It uses GPR for a couple of year by the implementation before ABI specifies FPR.
| @@ -2,7 +2,7 @@ | |||
| ; PR933 | |||
There was a problem hiding this comment.
There shouldn't be tests in test/CodeGen/X86 using the default target triple... probably the tests should be relocated or fixed.
There was a problem hiding this comment.
Handling of these tests here: #101944.
|
Could you please post an RFC on discourse for this (or link it if it already exists)? |
Made an attempt here: https://discourse.llvm.org/t/target-abi-improve-call-parameters-extensions-handling/80553. |
|
Any further comments on this, anyone? I guess I would like to commit the common-code parts, i.e. the introduction of the NoExt attribute, and keep the check in the SystemZ (disabled by default). Later on, maybe the check could be moved to the verifier. Does this seem reasonable? |
JonPsson1
force-pushed
the
VerifyIntArgs
branch
from
6f6eea4 to
4b7020d
Compare
|
Patch updated per @nikic suggestion to use a TargetOption flag to simply disable this for llc. With all the llc lit tests out of the way, I now see:
Not sure if these tools should also "do their job" and actually be fixed, like clang? Or are these also llc-like in the sense that they are only used internally by llvm developers? For the Objective-C test, not sure if this maybe could be disabled for SystemZ at least? These would pass with something like: |
|
Patch rebased. Tests failing as before: (still waiting for suggestions as to how to handle these failures: like llc (disabling) or do they need actual fixing?) |
|
The JIT tests should be fixed so that they reflect what a front-end developer should actually emit as LLVM IR. |
Do you mean this specifically for clang-repl, or all of them? Please give some more details (following from previous discussions here and on discourse - see link above). Thanks. |
There was a problem hiding this comment.
I think the general direction is fine here. I would suggest to split this up into three parts: 1. Add noext attribute and disabled verification, 2. fix any verification failures (e.g. by emitting noext), 3. enable verification.
It would be good to still be able to enable the verification in llc via an opt option, so you can write tests for it (and maybe useful when reducing an issue).
A bit hard to say without more information, but I think all of these should be fixed. Even if ObjC isn't really a thing on s390x, the root cause is probably legitimate (something emitting arguments without going through the proper API?) |
JonPsson1
force-pushed
the
VerifyIntArgs
branch
from
834d8bc to
10c997b
Compare
Patch updated per (1), meaning that it adds the noext attribute but with the actual verification disabled by default. Some llc tests are however added that use an overriding CL option '-argext-abi-check' to enable the verification. The verification has been improved to check that a function is not e.g. passed via ptr to an external function in which case it must adhere to the ABI even if its 'internal' (Internal functions are excluded for now but should not have to be, so that check could perhaps be removed before making this enabled by default.). By committing this first step
|
|
gentle ping to @nikic |
| @@ -1185,6 +1189,9 @@ Currently, only the following parameter attributes are defined: | |||
| value should be sign-extended to the extent required by the target's | |||
| ABI (which is usually 32-bits) by the caller (for a parameter) or | |||
| the callee (for a return value). | |||
| ``noext`` This indicates to the code generator that the parameter or return | |||
There was a problem hiding this comment.
Missing newline after attribute name.
| @@ -1185,6 +1189,9 @@ Currently, only the following parameter attributes are defined: | |||
| value should be sign-extended to the extent required by the target's | |||
| ABI (which is usually 32-bits) by the caller (for a parameter) or | |||
| the callee (for a return value). | |||
| ``noext`` This indicates to the code generator that the parameter or return | |||
| value has the high bits undefined, as for a struct in register, and | |||
| therefore does not need to be sign or zero extended. | |||
There was a problem hiding this comment.
The description here should say that this is the same as the default behavior, but some targets have additional validation to make sure one of the attributes is always present.
| // Only consider a function fully internal as long as it has local linkage | ||
| // and is not used in any other way than acting as the called function at | ||
| // call sites. TODO: Remove this when/if all internal functions adhere to | ||
| // the ABI. |
There was a problem hiding this comment.
We're generally free to change the ABI of internal functions, including in ways that do not match the system ABI. So I don't think this TODO can ever really be resolved.
There was a problem hiding this comment.
OK, removing that for now at least (not sure if this is actually the case on SystemZ that we would ever make special internal ABIs when lowering calls/returns).
| return; | ||
|
|
||
| // Temporarily only do the check when explicitly requested. | ||
| if (/* !getTargetMachine().Options.VerifyArgABICompliance && */ |
There was a problem hiding this comment.
Should have the TargetOptions check commented in even in this patch?
I think what you want to do is check getNumOccurrences() on the cl::opt and only use the value to override VerifyArgABICompliance if the option was explicitly passed.
There was a problem hiding this comment.
ok - using getNumCoccurrences() seems more clear, thanks. Hoping that it's ok to leave that logic in there with VerifyArgABICompliance by using a special extra temporary check against the CL option.
i8 test. LangRef text. tests IP Tests passing, mostly with -no-arg-exts NoExtend IP NoExt attribute instead tests at least passing ZeroExt flag instead of NoExt flag Updated was aab1ee8 IP Verify by default Fix in NoExt handling. SystemZ changes for ver was 02868e6 Rebase Try TargetOption was 4b7020d
JonPsson1
force-pushed
the
VerifyIntArgs
branch
from
10c997b to
a74fb7c
Compare
|
@nikic thanks for review - patch updated. |
|
@uweigand SystemZ parts ok? |
|
LGTM as well, thanks. |
For the purpose of verifying proper arguments extensions per the target's ABI, introduce the NoExt attribute that may be used by a target when neither sign- or zeroextension is required (e.g. with a struct in register). The purpose of doing so is to be able to verify that there is always one of these attributes present and by this detecting cases where sign/zero extension is actually missing. As a first step, this patch has the verification step done for the SystemZ backend only, but left off by default until all known issues have been addressed. Other targets/front-ends can now also add NoExt attribute where needed and do this check in the backend.
…rget A small step to ABIVerifier proposal disscussed in llvm#100757 (comment) Fixes: llvm#111406
The NoExt attribute was introduced with #100757, to exist alongside with signext and zeroext. This patch adds "noext" as an attribute to llvm-mode.el to get the proper highlighting of the keyword.
After a long delay, I am coming back to this patch due to recent interest. After rebasing, it seems that the old basic idea still works: introduce a NoExt attribute that can be added on narrow integers when neither sign- or zeroextension is required (typically "struct in reg"). Use that to detect cases of missing extension attributes.
This was last worked on (and begun) at https://reviews.llvm.org/D112942 where I took off from it to work on fixing this in some places that where then found to be broken (libcalls, sanitizers). There was also some discussion on #54530.
In summary:
Common-code:
On SystemZ:
At this point, the above seem to work and I have used clang to build SPEC and also LLVM itself without any cases of missing NoExt reported. So on SystemZ, this patch can already be applied to use the check of arguments (disabled with -no-argext-abi-check => patch NFC).
This has however generally been ignored in testing so ~400 (only a few of them fixed so far) tests have missing extension attributes which need to be fixed before this can be committed. I have built with -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;llvm;mlir" -DLLVM_ENABLE_RUNTIMES="openmp;compiler-rt", and run ninja; ninja check-all to get an overview of what would need to be fixed (I hope those are all the relevant subprojects..?)
SystemZ (CodeGen) tests. Either add 'internal' to functions, add extension attributes as needed, or pass -no-argext-abi-check to llc. Ideally I think extension attributes would be used everywhere, but at least for now maybe use the CL option for the hundreds of tests that have sofar ignored these argument attributes.
Generic (CodeGen) tests: don't use SystemZ CL option, so either add 'internal' to functions or the argument attributes.
Clang :: CodeGenObjC/blocks-ivar-debug.m
An ObjectiveC test that shows an i32 immediate arg without extension. Disable this test on SystemZ? (Apple only?)
Clang Interpreter tests are failing. These tests use clang-repl, and it seems that they emit functions without e.g. extending a returned i32. Seems like needs to be fixed.
Some failures with MLIR and libomp builds.
Perhaps this could be committed with the check turned off by default, and when all tests eventually have been updated (and any actual problems remaining have been fixed), it could be turned on by default..?