llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen
Author: None (yonghong-song)
<details>
<summary>Changes</summary>
Pu Lehui (pulehui@<!-- -->huaweicloud.com) reported an issue in private that at
no_alu32 mode clang may generate code which produced incorrect result with
riscv architecture.
The affected bpf prog is kfunc_call_test4 at bpf selftests
prog/kfunc_call_test.c. The following is the source code:
```
long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
int kfunc_call_test4(struct __sk_buff *skb)
{
...
tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
...
}
```
For the above code, at no_alu32 mode (-mcpu=v2), the asm code looks like
0: r1 = -3
1: r2 = -30
2: r3 = 0xffffff38 ll // opcode: 18 03 00 00 38 ff ff ff 00 00 00 00 00 00 00
00
4: r4 = -1000
5: call bpf_kfunc_call_test4
In bpf_kfunc_call_test4(), arguments with 'char', 'short' and 'long' are
generated correctly, for r1, r2 and r4 in the above. But the argument r3 is
generated with ld_imm64.
Further investigation is found that
- char/short type arguments are signed extended so naturally using MOV insn
- int type argument are zero extended so using ld_imm64 insn
- long type argument can do sign extension with 32-bit value so using MOV insn
In riscv case, the 'r3' value (0xffffff38 ll) will be passed to riscv kernel
code which does not do 32-bit sign extension and caused incorrect result.
Why intel/arm64 does not have this issue? x86_64/arm64 supports subrgisters so
for 'int' types, subregisters are directly used hence there is no issue.
Considering BPF is a 64-bit arch, so I think it makes sense at IR level 'int'
type argument should have the same sign-extension as 'char' or 'short' type.
This will solve the above riscv issue.
This patch will cause two codegen changes:
- for an 'int' constant argument, a MOV insn will be used instead of a
ld_imm64.
- for an 'int' register argument, for cpu=v1/v2, left/right shift will
happen. for cpu=v3/v4, there is no change from previous behavior as
subregisters will be used.
Tested with bpf selftests with all of no-alu32, cpu=v3 and cpu=v4, and all
passed.
---
Full diff: https://github.com/llvm/llvm-project/pull/84874.diff
3 Files Affected:
- (modified) clang/lib/CodeGen/Targets/BPF.cpp (+13)
- (modified) clang/test/CodeGen/bpf-abiinfo.c (+10)
- (added) llvm/test/CodeGen/BPF/cc_args_int.ll (+34)
``````````diff
diff --git a/clang/lib/CodeGen/Targets/BPF.cpp
b/clang/lib/CodeGen/Targets/BPF.cpp
index 2849222f7a1869..01937574779618 100644
--- a/clang/lib/CodeGen/Targets/BPF.cpp
+++ b/clang/lib/CodeGen/Targets/BPF.cpp
@@ -22,6 +22,19 @@ class BPFABIInfo : public DefaultABIInfo {
public:
BPFABIInfo(CodeGenTypes &CGT) : DefaultABIInfo(CGT) {}
+ bool isPromotableIntegerTypeForABI(QualType Ty) const {
+ if (ABIInfo::isPromotableIntegerTypeForABI(Ty) == true)
+ return true;
+
+ if (const auto *BT = Ty->getAs<BuiltinType>()) {
+ // For 'signed int' type, return true to allow sign-extension.
+ if (BT->getKind() == BuiltinType::Int)
+ return true;
+ }
+
+ return false;
+ }
+
ABIArgInfo classifyArgumentType(QualType Ty) const {
Ty = useFirstFieldIfTransparentUnion(Ty);
diff --git a/clang/test/CodeGen/bpf-abiinfo.c b/clang/test/CodeGen/bpf-abiinfo.c
index 366e8003f45572..6d259d8e6d6c73 100644
--- a/clang/test/CodeGen/bpf-abiinfo.c
+++ b/clang/test/CodeGen/bpf-abiinfo.c
@@ -22,3 +22,13 @@ int foo_int(void) {
if (bar_int() != 10) return 0; else return 1;
}
// CHECK: %call = call i32 @bar_int()
+
+void sprog1(short, int, int);
+void mprog1() {
+ sprog1(-3, 4, -5);
+// CHECK: call void @sprog1(i16 noundef signext -3, i32 noundef signext 4, i32
noundef signext -5)
+}
+void mprog2(long a, long b) {
+ sprog1(a, b, b);
+// CHECK: call void @sprog1(i16 noundef signext %{{[0-9a-z]+}}, i32 noundef
signext %{{[0-9a-z]+}}, i32 noundef signext %{{[0-9a-z]+}})
+}
diff --git a/llvm/test/CodeGen/BPF/cc_args_int.ll
b/llvm/test/CodeGen/BPF/cc_args_int.ll
new file mode 100644
index 00000000000000..79a9d27b87d709
--- /dev/null
+++ b/llvm/test/CodeGen/BPF/cc_args_int.ll
@@ -0,0 +1,34 @@
+; RUN: llc -march=bpfel -mcpu=v1 < %s | FileCheck --check-prefix=CHECK-V1 %s
+; RUN: llc -march=bpfel -mcpu=v2 < %s | FileCheck --check-prefix=CHECK-V2 %s
+; RUN: llc -march=bpfel -mcpu=v3 < %s | FileCheck --check-prefix=CHECK-V3 %s
+; RUN: llc -march=bpfel -mcpu=v4 < %s | FileCheck --check-prefix=CHECK-V4 %s
+
+declare dso_local void @bar(i16 noundef signext, i32 noundef signext, i32
noundef signext) local_unnamed_addr
+
+define void @test() {
+entry:
+ tail call void @bar(i16 noundef signext -3, i32 noundef signext 4, i32
noundef signext -5)
+; CHECK-V1: r2 = 4
+; CHECK-V1: r3 = -5
+; CHECK-V2: r2 = 4
+; CHECK-V2: r3 = -5
+; CHECK-V3: w2 = 4
+; CHECK-V3: w3 = -5
+; CHECK-V4: w2 = 4
+; CHECK-V4: w3 = -5
+ ret void
+}
+
+define dso_local void @test2(i64 noundef %a, i64 noundef %b)
local_unnamed_addr {
+entry:
+ %conv = trunc i64 %a to i16
+ %conv1 = trunc i64 %b to i32
+ tail call void @bar(i16 noundef signext %conv, i32 noundef signext %conv1,
i32 noundef signext %conv1)
+; CHECK-V1: r2 <<= 32
+; CHECK-V1: r2 s>>= 32
+; CHECK-V2: r2 <<= 32
+; CHECK-V2: r2 s>>= 32
+; CHECK-V3-NOT: r2 <<= 32
+; CHECK-V4-NOT: r2 <<= 32
+ ret void
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/84874
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
