[PATCH] D144829: [BPF] Add a few new insns under cpu=v4
Eduard Zingerman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 20 07:33:35 PDT 2023
eddyz87 added a comment.
I tried adding a test similar to `assemble-disassemble.ll`:
// RUN: llvm-mc -triple bpfel --mcpu=v4 --assemble --filetype=obj %s \
// RUN: | llvm-objdump -d --mattr=+alu32 - \
// RUN: | FileCheck %s
// CHECK: d7 01 00 00 10 00 00 00 r1 = bswap16 r1
// CHECK: d7 02 00 00 20 00 00 00 r2 = bswap32 r2
// CHECK: d7 03 00 00 40 00 00 00 r3 = bswap64 r3
r1 = bswap16 r1
r2 = bswap32 r2
r3 = bswap64 r3
// CHECK: 91 41 00 00 00 00 00 00 r1 = *(s8 *)(r4 + 0x0)
// CHECK: 89 52 04 00 00 00 00 00 r2 = *(s16 *)(r5 + 0x4)
// CHECK: 81 63 08 00 00 00 00 00 r3 = *(s32 *)(r6 + 0x8)
r1 = *(s8 *)(r4 + 0)
r2 = *(s16 *)(r5 + 4)
r3 = *(s32 *)(r6 + 8)
// CHECK: 91 41 00 00 00 00 00 00 w1 = *(s8 *)(r4 + 0x0)
// CHECK: 89 52 04 00 00 00 00 00 w2 = *(s16 *)(r5 + 0x4)
w1 = *(s8 *)(r4 + 0)
w2 = *(s16 *)(r5 + 4)
// CHECK: bf 41 08 00 00 00 00 00 r1 = (s8)r4
// CHECK: bf 52 10 00 00 00 00 00 r2 = (s16)r5
// CHECK: bf 63 20 00 00 00 00 00 r3 = (s32)w6
r1 = (s8)r4
r2 = (s16)r5
r3 = (s32)w6
// Should this work as well: r3 = (s32)r6 ?
// CHECK: bc 31 08 00 00 00 00 00 w1 = (s8)w3
// CHECK: bc 42 10 00 00 00 00 00 w2 = (s16)w4
w1 = (s8)w3
w2 = (s16)w4
And it looks like some instructions are not printed correctly:
$ llvm-mc -triple bpfel --mcpu=v4 --assemble --filetype=obj /home/eddy/work/llvm-project/llvm/test/CodeGen/BPF/assembler-disassembler-v4.s | llvm-objdump -d --mattr=+alu32 -
<stdin>: file format elf64-bpf
Disassembly of section .text:
0000000000000000 <.text>:
0: d7 01 00 00 10 00 00 00 r1 = bswap16 r1
1: d7 02 00 00 20 00 00 00 r2 = bswap32 r2
2: d7 03 00 00 40 00 00 00 r3 = bswap64 r3
3: 91 41 00 00 00 00 00 00 w1 = *(s8 *)(r4 + 0x0)
4: 89 52 04 00 00 00 00 00 w2 = *(s16 *)(r5 + 0x4)
5: 81 63 08 00 00 00 00 00 <unknown>
6: 91 41 00 00 00 00 00 00 w1 = *(s8 *)(r4 + 0x0)
7: 89 52 04 00 00 00 00 00 w2 = *(s16 *)(r5 + 0x4)
8: bf 41 08 00 00 00 00 00 r1 = (s8)r4
9: bf 52 10 00 00 00 00 00 r2 = (s16)r5
10: bf 63 20 00 00 00 00 00 r3 = (s32)w6
11: bc 31 08 00 00 00 00 00 w1 = (s8)w3
12: bc 42 10 00 00 00 00 00 w2 = (s16)w4
I'm not sure if this is an issue with disassembler or some additional `--mattr` options are needed.
================
Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:379
+ "$dst = (s8)$src",
+ [(set GPR:$dst, (sra (shl GPR:$src, (i64 56)), (i64 56)))]>;
+ def MOVSX_rr_16 : ALU_RR<BPF_ALU64, BPF_MOV, 16,
----------------
I think it is possible to avoid matching expansion pattern `(sra (shl GPR:$src, (i64 56))` here, and instead turn off the expansion when `movsx` is available.
I tried the change below and all BPF codegen tests are passing. Do I miss something?
---
```
diff --git a/llvm/lib/Target/BPF/BPFISelLowering.cpp b/llvm/lib/Target/BPF/BPFISelLowering.cpp
index 9a7357d6ad04..5e84af009591 100644
--- a/llvm/lib/Target/BPF/BPFISelLowering.cpp
+++ b/llvm/lib/Target/BPF/BPFISelLowering.cpp
@@ -132,9 +132,11 @@ BPFTargetLowering::BPFTargetLowering(const TargetMachine &TM,
setOperationAction(ISD::CTLZ_ZERO_UNDEF, MVT::i64, Custom);
setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i1, Expand);
- setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i8, Expand);
- setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i16, Expand);
- setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i32, Expand);
+ if (!STI.hasMovsx()) {
+ setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i8, Expand);
+ setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i16, Expand);
+ setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i32, Expand);
+ }
// Extended load operations for i1 types must be promoted
for (MVT VT : MVT::integer_valuetypes()) {
diff --git a/llvm/lib/Target/BPF/BPFInstrInfo.td b/llvm/lib/Target/BPF/BPFInstrInfo.td
index a1d532e60db2..29bec72aa92d 100644
--- a/llvm/lib/Target/BPF/BPFInstrInfo.td
+++ b/llvm/lib/Target/BPF/BPFInstrInfo.td
@@ -376,11 +376,11 @@ let Predicates = [BPFHasMovsx] in {
def MOVSX_rr_8 : ALU_RR<BPF_ALU64, BPF_MOV, 8,
(outs GPR:$dst), (ins GPR:$src),
"$dst = (s8)$src",
- [(set GPR:$dst, (sra (shl GPR:$src, (i64 56)), (i64 56)))]>;
+ [(set GPR:$dst, (sext_inreg GPR:$src, i8))]>;
def MOVSX_rr_16 : ALU_RR<BPF_ALU64, BPF_MOV, 16,
(outs GPR:$dst), (ins GPR:$src),
"$dst = (s16)$src",
- [(set GPR:$dst, (sra (shl GPR:$src, (i64 48)), (i64 48)))]>;
+ [(set GPR:$dst, (sext_inreg GPR:$src, i16))]>;
def MOVSX_rr_32 : ALU_RR<BPF_ALU64, BPF_MOV, 32,
(outs GPR:$dst), (ins GPR32:$src),
"$dst = (s32)$src",
@@ -388,11 +388,11 @@ let Predicates = [BPFHasMovsx] in {
def MOVSX_rr_32_8 : ALU_RR<BPF_ALU, BPF_MOV, 8,
(outs GPR32:$dst), (ins GPR32:$src),
"$dst = (s8)$src",
- [(set GPR32:$dst, (sra (shl GPR32:$src, (i32 24)), (i32 24)))]>;
+ [(set GPR32:$dst, (sext_inreg GPR32:$src, i8))]>;
def MOVSX_rr_32_16 : ALU_RR<BPF_ALU, BPF_MOV, 16,
(outs GPR32:$dst), (ins GPR32:$src),
"$dst = (s16)$src",
- [(set GPR32:$dst, (sra (shl GPR32:$src, (i32 16)), (i32 16)))]>;
+ [(set GPR32:$dst, (sext_inreg GPR32:$src, i16))]>;
}
}
```
================
Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:321
+
+ std::map<unsigned, unsigned> ReverseCondOpMap;
----------------
Is this map unused?
================
Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:412
+ int CurrNumInsns = 0;
+ std::map<MachineBasicBlock *, int> SoFarNumInsns;
+ std::map<MachineBasicBlock *, MachineBasicBlock *> FollowThroughBB;
----------------
Nitpick: Fangrui suggested in my llvm-objdump revisions to use `DenseMap` in most cases (as `std::map` allocates for each pair).
================
Comment at: llvm/test/CodeGen/BPF/movsx.ll:30
+}
+; CHECK: w0 = w1 # encoding: [0xbc,0x10,0x00,0x00,0x00,0x00,0x00,0x00]
+
----------------
This does not seem right, as it does not sign extend 8-bit argument to 16-bit value.
================
Comment at: llvm/test/CodeGen/BPF/movsx.ll:38
+}
+; CHECK: w0 = w1 # encoding: [0xbc,0x10,0x00,0x00,0x00,0x00,0x00,0x00]
+
----------------
Shouldn't this be `w0 = (s8)w1`?
A few checks below also look strange.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144829/new/
https://reviews.llvm.org/D144829
More information about the cfe-commits
mailing list