All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH LLVM 0/3] bpf: improvements on XADD semantics check and code-gen
@ 2019-02-27 20:50 Jiong Wang
  2019-02-27 20:50 ` [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD Jiong Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Jiong Wang @ 2019-02-27 20:50 UTC (permalink / raw)
  To: alexei.starovoitov, yhs; +Cc: daniel, bpf, oss-drivers, Jiong Wang

BPF XADD semantics require all Defs of XADD are dead, meaning any result of
XADD insn is not used.

However, BPF backend hasn't enabled sub-register liveness track, so when
the source and destination operands of XADD are GPR32, there is no
sub-register dead info. If we rely on the generic
MachineInstr::allDefsAreDead, then we will raise false alarm on GPR32 Def.

This was fine as there was no sub-register code-gen support for XADD, but
it will be added by the second patch. Without proper handling of GPR32 Def,
testcase like the one added in the second patch will fail on sub-register
code-gen mode.

To support GPR32 Def, ideally we could just enable sub-registr liveness
track on BPF backend, then allDefsAreDead could work on GPR32 Def. This
requires implementing TargetSubtargetInfo::enableSubRegLiveness on BPF.

However, sub-register liveness tracking module inside LLVM is actually
designed for the situation where one register could be splitted into more
than one sub-registers for which case each sub-register could have their
own liveness and kill one of them doesn't kill others. So, tracking
liveness for each make sense.

For BPF, each 64-bit register could only have one 32-bit sub-register. This
is exactly the case which LLVM think brings no benefits for doing
sub-register tracking, because the live range of sub-register must always
equal to its parent register, therefore liveness tracking is disabled even
the back-end has implemented enableSubRegLiveness. The detailed information
is at r232695:

  Author: Matthias Braun <matze@braunis.de>
  Date:   Thu Mar 19 00:21:58 2015 +0000
  Do not track subregister liveness when it brings no benefits

Hence, for BPF, we enhance MachineInstr::allDefsAreDead. Given the solo
sub-register always has the same liveness as its parent register, LLVM is
already attaching a implicit 64-bit register Def whenever the there is
a sub-register Def. The liveness of the implicit 64-bit Def is available.
For example, for "lock *(u32 *)(r0 + 4) += w9", the MachineOperand info
could be:

  $w9 = XADDW32 killed $r0, 4, $w9(tied-def 0),
                       implicit killed $r9, implicit-def dead $r9

Even though w9 is not marked as Dead, the parent register r9 is marked as
Dead correctly, and it is safe to use such information or our purpose.

After above improvements, the second patch in this set then adds
sub-register code-gen support for XADD.

Jiong Wang (3):
  bpf: improve dead Defs check for XADD
  bpf: enable sub-register code-gen for XADD
  bpf: disassembler support for XADD under sub-register mode

 lib/Target/BPF/BPFInstrInfo.td                  |  28 ++++++-
 lib/Target/BPF/BPFMIChecking.cpp                | 107 +++++++++++++++++++++++-
 lib/Target/BPF/Disassembler/BPFDisassembler.cpp |   3 +-
 test/CodeGen/BPF/xadd.ll                        |   2 +
 test/CodeGen/BPF/xadd_legal.ll                  |  26 ++++++
 test/MC/BPF/load-store-32.s                     |   3 +
 6 files changed, 162 insertions(+), 7 deletions(-)
 create mode 100644 test/CodeGen/BPF/xadd_legal.ll

-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD
  2019-02-27 20:50 [PATCH LLVM 0/3] bpf: improvements on XADD semantics check and code-gen Jiong Wang
@ 2019-02-27 20:50 ` Jiong Wang
  2019-02-28  6:46   ` Yonghong Song
  2019-02-27 20:50 ` [PATCH LLVM 2/3] bpf: enable sub-register code-gen " Jiong Wang
  2019-02-27 20:50 ` [PATCH LLVM 3/3] bpf: disassembler support for XADD under sub-register mode Jiong Wang
  2 siblings, 1 reply; 9+ messages in thread
From: Jiong Wang @ 2019-02-27 20:50 UTC (permalink / raw)
  To: alexei.starovoitov, yhs; +Cc: daniel, bpf, oss-drivers, Jiong Wang

BPF XADD semantics require all Defs of XADD are dead, meaning any result of
XADD insn is not used.

However, BPF backend hasn't enabled sub-register liveness track, so when
the source and destination operands of XADD are GPR32, there is no
sub-register dead info. If we rely on the generic
MachineInstr::allDefsAreDead, then we will raise false alarm on GPR32 Def.
This was fine as there was no sub-register code-gen support for XADD which
will be added by the next patch.

To support GPR32 Def, ideally we could just enable sub-registr liveness
track on BPF backend, then allDefsAreDead could work on GPR32 Def. This
requires implementing TargetSubtargetInfo::enableSubRegLiveness on BPF.

However, sub-register liveness tracking module inside LLVM is actually
designed for the situation where one register could be splitted into more
than one sub-registers for which case each sub-register could have their
own liveness and kill one of them doesn't kill others. So, tracking
liveness for each make sense.

For BPF, each 64-bit register could only have one 32-bit sub-register. This
is exactly the case which LLVM think brings no benefits for doing
sub-register tracking, because the live range of sub-register must always
equal to its parent register, therefore liveness tracking is disabled even
the back-end has implemented enableSubRegLiveness. The detailed information
is at r232695:

  Author: Matthias Braun <matze@braunis.de>
  Date:   Thu Mar 19 00:21:58 2015 +0000
  Do not track subregister liveness when it brings no benefits

Hence, for BPF, we enhance MachineInstr::allDefsAreDead. Given the solo
sub-register always has the same liveness as its parent register, LLVM is
already attaching a implicit 64-bit register Def whenever the there is
a sub-register Def. The liveness of the implicit 64-bit Def is available.
For example, for "lock *(u32 *)(r0 + 4) += w9", the MachineOperand info
could be:

  $w9 = XADDW32 killed $r0, 4, $w9(tied-def 0),
                       implicit killed $r9, implicit-def dead $r9

Even though w9 is not marked as Dead, the parent register r9 is marked as
Dead correctly, and it is safe to use such information or our purpose.

Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 lib/Target/BPF/BPFMIChecking.cpp | 103 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/lib/Target/BPF/BPFMIChecking.cpp b/lib/Target/BPF/BPFMIChecking.cpp
index a1e81cb..7a4ba96 100644
--- a/lib/Target/BPF/BPFMIChecking.cpp
+++ b/lib/Target/BPF/BPFMIChecking.cpp
@@ -61,6 +61,107 @@ void BPFMIPreEmitChecking::initialize(MachineFunction &MFParm) {
   LLVM_DEBUG(dbgs() << "*** BPF PreEmit checking pass ***\n\n");
 }
 
+// Make sure all Defs of XADD are dead, meaning any result of XADD insn is not
+// used.
+//
+// NOTE: BPF backend hasn't enabled sub-register liveness track, so when the
+// source and destination operands of XADD are GPR32, there is no sub-register
+// dead info. If we rely on the generic MachineInstr::allDefsAreDead, then we
+// will raise false alarm on GPR32 Def.
+//
+// To support GPR32 Def, ideally we could just enable sub-registr liveness track
+// on BPF backend, then allDefsAreDead could work on GPR32 Def. This requires
+// implementing TargetSubtargetInfo::enableSubRegLiveness on BPF.
+//
+// However, sub-register liveness tracking module inside LLVM is actually
+// designed for the situation where one register could be splitted into more
+// than one sub-registers for which case each sub-register could have their own
+// liveness and kill one of them doesn't kill others. So, tracking liveness for
+// each make sense.
+//
+// For BPF, each 64-bit register could only have one 32-bit sub-register. This
+// is exactly the case which LLVM think brings no benefits for doing
+// sub-register tracking, because the live range of sub-register must always
+// equal to its parent register, therefore liveness tracking is disabled even
+// the back-end has implemented enableSubRegLiveness. The detailed information
+// is at r232695:
+//
+//   Author: Matthias Braun <matze@braunis.de>
+//   Date:   Thu Mar 19 00:21:58 2015 +0000
+//   Do not track subregister liveness when it brings no benefits
+//
+// Hence, for BPF, we enhance MachineInstr::allDefsAreDead. Given the solo
+// sub-register always has the same liveness as its parent register, LLVM is
+// already attaching a implicit 64-bit register Def whenever the there is
+// a sub-register Def. The liveness of the implicit 64-bit Def is available.
+// For example, for "lock *(u32 *)(r0 + 4) += w9", the MachineOperand info could
+// be:
+//
+//   $w9 = XADDW32 killed $r0, 4, $w9(tied-def 0),
+//                        implicit killed $r9, implicit-def dead $r9
+//
+// Even though w9 is not marked as Dead, the parent register r9 is marked as
+// Dead correctly, and it is safe to use such information or our purpose.
+static bool hasLivingDefs(const MachineInstr &MI,
+                          const TargetRegisterInfo *TRI) {
+  const MCRegisterClass *GPR64RegClass =
+    &BPFMCRegisterClasses[BPF::GPRRegClassID];
+  std::vector<unsigned> GPR32LiveDefs;
+  std::vector<unsigned> GPR64DeadDefs;
+
+  for (const MachineOperand &MO : MI.operands()) {
+    bool RegIsGPR64;
+
+    if (!MO.isReg() || MO.isUse())
+      continue;
+
+    RegIsGPR64 = GPR64RegClass->contains(MO.getReg());
+    if (!MO.isDead()) {
+      // It is a GPR64 live Def, we are sure it is live. */
+      if (RegIsGPR64)
+        return true;
+      // It is a GPR32 live Def, we are unsure whether it is really dead due to
+      // no sub-register liveness tracking. Push it to vector for deferred
+      // check.
+      GPR32LiveDefs.push_back(MO.getReg());
+      continue;
+    }
+
+    // Record any GPR64 dead Def as some unmarked GPR32 could be alias of its
+    // low 32-bit.
+    if (RegIsGPR64)
+      GPR64DeadDefs.push_back(MO.getReg());
+  }
+
+  // No GPR32 live Def, safe to return false.
+  if (GPR32LiveDefs.empty())
+    return false;
+
+  // No GPR64 dead Def, so all those GPR32 live Def can't have alias, therefore
+  // must be truely live, safe to return true.
+  if (GPR64DeadDefs.empty())
+    return true;
+
+  // Otherwise, return true if any aliased SuperReg of GPR32 is not dead.
+  std::vector<unsigned>::iterator search_begin = GPR64DeadDefs.begin();
+  std::vector<unsigned>::iterator search_end = GPR64DeadDefs.end();
+  for (auto I : GPR32LiveDefs) {
+    bool ActuallyDead = true;
+
+    for (MCSuperRegIterator SR(I, TRI); SR.isValid(); ++SR) {
+       if (std::find(search_begin, search_end, *SR) == search_end) {
+         ActuallyDead = false;
+         break;
+       }
+    }
+
+    if (!ActuallyDead)
+      return true;
+  }
+
+  return false;
+}
+
 void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
   for (MachineBasicBlock &MBB : *MF) {
     for (MachineInstr &MI : MBB) {
@@ -68,7 +169,7 @@ void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
         continue;
 
       LLVM_DEBUG(MI.dump());
-      if (!MI.allDefsAreDead()) {
+      if (hasLivingDefs(MI, TRI)) {
         DebugLoc Empty;
         const DebugLoc &DL = MI.getDebugLoc();
         if (DL != Empty)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH LLVM 2/3] bpf: enable sub-register code-gen for XADD
  2019-02-27 20:50 [PATCH LLVM 0/3] bpf: improvements on XADD semantics check and code-gen Jiong Wang
  2019-02-27 20:50 ` [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD Jiong Wang
@ 2019-02-27 20:50 ` Jiong Wang
  2019-02-28  6:51   ` Yonghong Song
  2019-02-27 20:50 ` [PATCH LLVM 3/3] bpf: disassembler support for XADD under sub-register mode Jiong Wang
  2 siblings, 1 reply; 9+ messages in thread
From: Jiong Wang @ 2019-02-27 20:50 UTC (permalink / raw)
  To: alexei.starovoitov, yhs; +Cc: daniel, bpf, oss-drivers, Jiong Wang

Support sub-register code-gen for XADD is like supporting any other Load
and Store patterns.

No new instruction is introduced.

  lock *(u32 *)(r1 + 0) += w2

has exactly the same underlying insn as:

  lock *(u32 *)(r1 + 0) += r2

BPF_W width modifier has guaranteed they behave the same at runtime. This
patch merely teaches BPF back-end that BPF_W width modifier could work
GPR32 register class and that's all needed for sub-register code-gen
support for XADD.

test/CodeGen/BPF/xadd.ll updated to include sub-register code-gen tests.

A new testcase test/CodeGen/BPF/xadd_legal.ll is added to make sure the
legal case could pass on all code-gen modes. It could also test dead Def
check on GPR32. If there is no proper handling like what has been done
inside BPFMIChecking.cpp:hasLivingDefs, then this testcase will fail.

Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 lib/Target/BPF/BPFInstrInfo.td   | 28 ++++++++++++++++++++++++----
 lib/Target/BPF/BPFMIChecking.cpp |  4 +++-
 test/CodeGen/BPF/xadd.ll         |  2 ++
 test/CodeGen/BPF/xadd_legal.ll   | 26 ++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 5 deletions(-)
 create mode 100644 test/CodeGen/BPF/xadd_legal.ll

diff --git a/lib/Target/BPF/BPFInstrInfo.td b/lib/Target/BPF/BPFInstrInfo.td
index 36724b8..c44702a 100644
--- a/lib/Target/BPF/BPFInstrInfo.td
+++ b/lib/Target/BPF/BPFInstrInfo.td
@@ -614,11 +614,31 @@ class XADD<BPFWidthModifer SizeOp, string OpcodeStr, PatFrag OpNode>
   let BPFClass = BPF_STX;
 }
 
+class XADD32<BPFWidthModifer SizeOp, string OpcodeStr, PatFrag OpNode>
+    : TYPE_LD_ST<BPF_XADD.Value, SizeOp.Value,
+                 (outs GPR32:$dst),
+                 (ins MEMri:$addr, GPR32:$val),
+                 "lock *("#OpcodeStr#" *)($addr) += $val",
+                 [(set GPR32:$dst, (OpNode ADDRri:$addr, GPR32:$val))]> {
+  bits<4> dst;
+  bits<20> addr;
+
+  let Inst{51-48} = addr{19-16}; // base reg
+  let Inst{55-52} = dst;
+  let Inst{47-32} = addr{15-0}; // offset
+  let BPFClass = BPF_STX;
+}
+
 let Constraints = "$dst = $val" in {
-def XADD32 : XADD<BPF_W, "u32", atomic_load_add_32>;
-def XADD64 : XADD<BPF_DW, "u64", atomic_load_add_64>;
-// undefined def XADD16 : XADD<1, "xadd16", atomic_load_add_16>;
-// undefined def XADD8  : XADD<2, "xadd8", atomic_load_add_8>;
+  let Predicates = [BPFNoALU32] in {
+    def XADDW : XADD<BPF_W, "u32", atomic_load_add_32>;
+  }
+
+  let Predicates = [BPFHasALU32], DecoderNamespace = "BPFALU32" in {
+    def XADDW32 : XADD32<BPF_W, "u32", atomic_load_add_32>;
+  }
+
+  def XADDD : XADD<BPF_DW, "u64", atomic_load_add_64>;
 }
 
 // bswap16, bswap32, bswap64
diff --git a/lib/Target/BPF/BPFMIChecking.cpp b/lib/Target/BPF/BPFMIChecking.cpp
index 7a4ba96..938e539 100644
--- a/lib/Target/BPF/BPFMIChecking.cpp
+++ b/lib/Target/BPF/BPFMIChecking.cpp
@@ -165,7 +165,9 @@ static bool hasLivingDefs(const MachineInstr &MI,
 void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
   for (MachineBasicBlock &MBB : *MF) {
     for (MachineInstr &MI : MBB) {
-      if (MI.getOpcode() != BPF::XADD32 && MI.getOpcode() != BPF::XADD64)
+      if (MI.getOpcode() != BPF::XADDW &&
+          MI.getOpcode() != BPF::XADDD &&
+          MI.getOpcode() != BPF::XADDW32)
         continue;
 
       LLVM_DEBUG(MI.dump());
diff --git a/test/CodeGen/BPF/xadd.ll b/test/CodeGen/BPF/xadd.ll
index c5f2620..49f0c10 100644
--- a/test/CodeGen/BPF/xadd.ll
+++ b/test/CodeGen/BPF/xadd.ll
@@ -1,5 +1,7 @@
 ; RUN: not llc -march=bpfel < %s 2>&1 | FileCheck %s
 ; RUN: not llc -march=bpfeb < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=bpfel -mattr=+alu32 < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=bpfeb -mattr=+alu32 < %s 2>&1 | FileCheck %s
 
 ; This file is generated with the source command and source
 ; $ clang -target bpf -O2 -g -S -emit-llvm t.c
diff --git a/test/CodeGen/BPF/xadd_legal.ll b/test/CodeGen/BPF/xadd_legal.ll
new file mode 100644
index 0000000..0d30084
--- /dev/null
+++ b/test/CodeGen/BPF/xadd_legal.ll
@@ -0,0 +1,26 @@
+; RUN: llc -march=bpfel < %s 2>&1 | FileCheck --check-prefix=CHECK-64 %s
+; RUN: llc -march=bpfeb < %s 2>&1 | FileCheck --check-prefix=CHECK-64 %s
+; RUN: llc -march=bpfel -mattr=+alu32 < %s 2>&1 | FileCheck --check-prefix=CHECK-32 %s
+; RUN: llc -march=bpfeb -mattr=+alu32 < %s 2>&1 | FileCheck --check-prefix=CHECK-32 %s
+
+; This file is generated with the source command and source
+; $ clang -target bpf -O2 -S -emit-llvm t.c
+; $ cat t.c
+; int test(int *ptr, unsigned long long a) {
+;    __sync_fetch_and_add(ptr, a);
+;    return *ptr;
+; }
+;
+; NOTE: passing unsigned long long as the second operand of __sync_fetch_and_add
+; could effectively create sub-register reference coming from indexing a full
+; register which could then exerceise hasLivingDefs inside BPFMIChecker.cpp.
+
+define dso_local i32 @test(i32* nocapture %ptr, i64 %a) {
+entry:
+  %conv = trunc i64 %a to i32
+  %0 = atomicrmw add i32* %ptr, i32 %conv seq_cst
+; CHECK-64: lock *(u32 *)(r1 + 0) += r2
+; CHECK-32: lock *(u32 *)(r1 + 0) += w2
+  %1 = load i32, i32* %ptr, align 4
+  ret i32 %1
+}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH LLVM 3/3] bpf: disassembler support for XADD under sub-register mode
  2019-02-27 20:50 [PATCH LLVM 0/3] bpf: improvements on XADD semantics check and code-gen Jiong Wang
  2019-02-27 20:50 ` [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD Jiong Wang
  2019-02-27 20:50 ` [PATCH LLVM 2/3] bpf: enable sub-register code-gen " Jiong Wang
@ 2019-02-27 20:50 ` Jiong Wang
  2019-02-28  6:54   ` Yonghong Song
  2 siblings, 1 reply; 9+ messages in thread
From: Jiong Wang @ 2019-02-27 20:50 UTC (permalink / raw)
  To: alexei.starovoitov, yhs; +Cc: daniel, bpf, oss-drivers, Jiong Wang

Like the other load/store instructions, "w" register is prefered when
disassembling BPF_STX | BPF_W | BPF_XADD under sub-register mode.

Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 lib/Target/BPF/Disassembler/BPFDisassembler.cpp | 3 ++-
 test/MC/BPF/load-store-32.s                     | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/Target/BPF/Disassembler/BPFDisassembler.cpp b/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
index ed09e44..c5be7cb 100644
--- a/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
+++ b/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
@@ -171,9 +171,10 @@ DecodeStatus BPFDisassembler::getInstruction(MCInst &Instr, uint64_t &Size,
   if (Result == MCDisassembler::Fail) return MCDisassembler::Fail;
 
   uint8_t InstClass = getInstClass(Insn);
+  uint8_t InstMode = getInstMode(Insn);
   if ((InstClass == BPF_LDX || InstClass == BPF_STX) &&
       getInstSize(Insn) != BPF_DW &&
-      getInstMode(Insn) == BPF_MEM &&
+      (InstMode == BPF_MEM || InstMode == BPF_XADD) &&
       STI.getFeatureBits()[BPF::ALU32])
     Result = decodeInstruction(DecoderTableBPFALU3264, Instr, Insn, Address,
                                this, STI);
diff --git a/test/MC/BPF/load-store-32.s b/test/MC/BPF/load-store-32.s
index 73ed9fc..be576d2 100644
--- a/test/MC/BPF/load-store-32.s
+++ b/test/MC/BPF/load-store-32.s
@@ -17,9 +17,12 @@
   *(u8 *)(r0 + 0) = w7    // BPF_STX | BPF_B
   *(u16 *)(r1 + 8) = w8   // BPF_STX | BPF_H
   *(u32 *)(r2 + 16) = w9  // BPF_STX | BPF_W
+  lock *(u32 *)(r2 + 16) += w9  // BPF_STX | BPF_W | BPF_XADD
 // CHECK-32: 73 70 00 00 00 00 00 00 	*(u8 *)(r0 + 0) = w7
 // CHECK-32: 6b 81 08 00 00 00 00 00 	*(u16 *)(r1 + 8) = w8
 // CHECK-32: 63 92 10 00 00 00 00 00 	*(u32 *)(r2 + 16) = w9
+// CHECK-32: c3 92 10 00 00 00 00 00 	lock *(u32 *)(r2 + 16) += w9
 // CHECK: 73 70 00 00 00 00 00 00 	*(u8 *)(r0 + 0) = r7
 // CHECK: 6b 81 08 00 00 00 00 00 	*(u16 *)(r1 + 8) = r8
 // CHECK: 63 92 10 00 00 00 00 00 	*(u32 *)(r2 + 16) = r9
+// CHECK: c3 92 10 00 00 00 00 00 	lock *(u32 *)(r2 + 16) += r9
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD
  2019-02-27 20:50 ` [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD Jiong Wang
@ 2019-02-28  6:46   ` Yonghong Song
  2019-02-28  8:20     ` Jiong Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Yonghong Song @ 2019-02-28  6:46 UTC (permalink / raw)
  To: Jiong Wang, alexei.starovoitov; +Cc: daniel, bpf, oss-drivers



On 2/27/19 12:50 PM, Jiong Wang wrote:
> BPF XADD semantics require all Defs of XADD are dead, meaning any result of
> XADD insn is not used.
> 
> However, BPF backend hasn't enabled sub-register liveness track, so when
> the source and destination operands of XADD are GPR32, there is no
> sub-register dead info. If we rely on the generic
> MachineInstr::allDefsAreDead, then we will raise false alarm on GPR32 Def.
> This was fine as there was no sub-register code-gen support for XADD which
> will be added by the next patch.
> 
> To support GPR32 Def, ideally we could just enable sub-registr liveness
> track on BPF backend, then allDefsAreDead could work on GPR32 Def. This
> requires implementing TargetSubtargetInfo::enableSubRegLiveness on BPF.
> 
> However, sub-register liveness tracking module inside LLVM is actually
> designed for the situation where one register could be splitted into more
> than one sub-registers for which case each sub-register could have their
> own liveness and kill one of them doesn't kill others. So, tracking
> liveness for each make sense.
> 
> For BPF, each 64-bit register could only have one 32-bit sub-register. This
> is exactly the case which LLVM think brings no benefits for doing
> sub-register tracking, because the live range of sub-register must always
> equal to its parent register, therefore liveness tracking is disabled even
> the back-end has implemented enableSubRegLiveness. The detailed information
> is at r232695:
> 
>    Author: Matthias Braun <matze@braunis.de>
>    Date:   Thu Mar 19 00:21:58 2015 +0000
>    Do not track subregister liveness when it brings no benefits
> 
> Hence, for BPF, we enhance MachineInstr::allDefsAreDead. Given the solo
> sub-register always has the same liveness as its parent register, LLVM is
> already attaching a implicit 64-bit register Def whenever the there is
> a sub-register Def. The liveness of the implicit 64-bit Def is available.
> For example, for "lock *(u32 *)(r0 + 4) += w9", the MachineOperand info
> could be:
> 
>    $w9 = XADDW32 killed $r0, 4, $w9(tied-def 0),
>                         implicit killed $r9, implicit-def dead $r9
> 
> Even though w9 is not marked as Dead, the parent register r9 is marked as
> Dead correctly, and it is safe to use such information or our purpose.
> 
> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
> ---
>   lib/Target/BPF/BPFMIChecking.cpp | 103 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/Target/BPF/BPFMIChecking.cpp b/lib/Target/BPF/BPFMIChecking.cpp
> index a1e81cb..7a4ba96 100644
> --- a/lib/Target/BPF/BPFMIChecking.cpp
> +++ b/lib/Target/BPF/BPFMIChecking.cpp
> @@ -61,6 +61,107 @@ void BPFMIPreEmitChecking::initialize(MachineFunction &MFParm) {
>     LLVM_DEBUG(dbgs() << "*** BPF PreEmit checking pass ***\n\n");
>   }
>   
> +// Make sure all Defs of XADD are dead, meaning any result of XADD insn is not
> +// used.
> +//
> +// NOTE: BPF backend hasn't enabled sub-register liveness track, so when the
> +// source and destination operands of XADD are GPR32, there is no sub-register
> +// dead info. If we rely on the generic MachineInstr::allDefsAreDead, then we
> +// will raise false alarm on GPR32 Def.
> +//
> +// To support GPR32 Def, ideally we could just enable sub-registr liveness track
> +// on BPF backend, then allDefsAreDead could work on GPR32 Def. This requires
> +// implementing TargetSubtargetInfo::enableSubRegLiveness on BPF.
> +//
> +// However, sub-register liveness tracking module inside LLVM is actually
> +// designed for the situation where one register could be splitted into more
> +// than one sub-registers for which case each sub-register could have their own
> +// liveness and kill one of them doesn't kill others. So, tracking liveness for
> +// each make sense.
> +//
> +// For BPF, each 64-bit register could only have one 32-bit sub-register. This
> +// is exactly the case which LLVM think brings no benefits for doing
> +// sub-register tracking, because the live range of sub-register must always
> +// equal to its parent register, therefore liveness tracking is disabled even
> +// the back-end has implemented enableSubRegLiveness. The detailed information
> +// is at r232695:
> +//
> +//   Author: Matthias Braun <matze@braunis.de>
> +//   Date:   Thu Mar 19 00:21:58 2015 +0000
> +//   Do not track subregister liveness when it brings no benefits
> +//
> +// Hence, for BPF, we enhance MachineInstr::allDefsAreDead. Given the solo
> +// sub-register always has the same liveness as its parent register, LLVM is
> +// already attaching a implicit 64-bit register Def whenever the there is
> +// a sub-register Def. The liveness of the implicit 64-bit Def is available.
> +// For example, for "lock *(u32 *)(r0 + 4) += w9", the MachineOperand info could
> +// be:
> +//
> +//   $w9 = XADDW32 killed $r0, 4, $w9(tied-def 0),
> +//                        implicit killed $r9, implicit-def dead $r9
> +//
> +// Even though w9 is not marked as Dead, the parent register r9 is marked as
> +// Dead correctly, and it is safe to use such information or our purpose.
> +static bool hasLivingDefs(const MachineInstr &MI,
> +                          const TargetRegisterInfo *TRI) {
> +  const MCRegisterClass *GPR64RegClass =
> +    &BPFMCRegisterClasses[BPF::GPRRegClassID];
> +  std::vector<unsigned> GPR32LiveDefs;
> +  std::vector<unsigned> GPR64DeadDefs;
> +
> +  for (const MachineOperand &MO : MI.operands()) {
> +    bool RegIsGPR64;
> +
> +    if (!MO.isReg() || MO.isUse())
> +      continue;
> +
> +    RegIsGPR64 = GPR64RegClass->contains(MO.getReg());
> +    if (!MO.isDead()) {
> +      // It is a GPR64 live Def, we are sure it is live. */
> +      if (RegIsGPR64)
> +        return true;
> +      // It is a GPR32 live Def, we are unsure whether it is really dead due to
> +      // no sub-register liveness tracking. Push it to vector for deferred
> +      // check.
> +      GPR32LiveDefs.push_back(MO.getReg());
> +      continue;
> +    }
> +
> +    // Record any GPR64 dead Def as some unmarked GPR32 could be alias of its
> +    // low 32-bit.
> +    if (RegIsGPR64)
> +      GPR64DeadDefs.push_back(MO.getReg());
> +  }
> +
> +  // No GPR32 live Def, safe to return false.
> +  if (GPR32LiveDefs.empty())
> +    return false;
> +
> +  // No GPR64 dead Def, so all those GPR32 live Def can't have alias, therefore
> +  // must be truely live, safe to return true.
> +  if (GPR64DeadDefs.empty())
> +    return true;
> +
> +  // Otherwise, return true if any aliased SuperReg of GPR32 is not dead.
> +  std::vector<unsigned>::iterator search_begin = GPR64DeadDefs.begin();
> +  std::vector<unsigned>::iterator search_end = GPR64DeadDefs.end();
> +  for (auto I : GPR32LiveDefs) {
> +    bool ActuallyDead = true;
> +
> +    for (MCSuperRegIterator SR(I, TRI); SR.isValid(); ++SR) {
> +       if (std::find(search_begin, search_end, *SR) == search_end) {

maybe you can just return true here?

> +         ActuallyDead = false;
> +         break;
> +       }
> +    }
> +
> +    if (!ActuallyDead)
> +      return true;
> +  }
> +
> +  return false;
> +}
> +
>   void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
>     for (MachineBasicBlock &MBB : *MF) {
>       for (MachineInstr &MI : MBB) {
> @@ -68,7 +169,7 @@ void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
>           continue;
>   
>         LLVM_DEBUG(MI.dump());
> -      if (!MI.allDefsAreDead()) {
> +      if (hasLivingDefs(MI, TRI)) {
>           DebugLoc Empty;
>           const DebugLoc &DL = MI.getDebugLoc();
>           if (DL != Empty)
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH LLVM 2/3] bpf: enable sub-register code-gen for XADD
  2019-02-27 20:50 ` [PATCH LLVM 2/3] bpf: enable sub-register code-gen " Jiong Wang
@ 2019-02-28  6:51   ` Yonghong Song
  0 siblings, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2019-02-28  6:51 UTC (permalink / raw)
  To: Jiong Wang, alexei.starovoitov; +Cc: daniel, bpf, oss-drivers



On 2/27/19 12:50 PM, Jiong Wang wrote:
> Support sub-register code-gen for XADD is like supporting any other Load
> and Store patterns.
> 
> No new instruction is introduced.
> 
>    lock *(u32 *)(r1 + 0) += w2
> 
> has exactly the same underlying insn as:
> 
>    lock *(u32 *)(r1 + 0) += r2
> 
> BPF_W width modifier has guaranteed they behave the same at runtime. This
> patch merely teaches BPF back-end that BPF_W width modifier could work
> GPR32 register class and that's all needed for sub-register code-gen
> support for XADD.
> 
> test/CodeGen/BPF/xadd.ll updated to include sub-register code-gen tests.
> 
> A new testcase test/CodeGen/BPF/xadd_legal.ll is added to make sure the
> legal case could pass on all code-gen modes. It could also test dead Def
> check on GPR32. If there is no proper handling like what has been done
> inside BPFMIChecking.cpp:hasLivingDefs, then this testcase will fail.
> 
> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
> ---
>   lib/Target/BPF/BPFInstrInfo.td   | 28 ++++++++++++++++++++++++----
>   lib/Target/BPF/BPFMIChecking.cpp |  4 +++-
>   test/CodeGen/BPF/xadd.ll         |  2 ++
>   test/CodeGen/BPF/xadd_legal.ll   | 26 ++++++++++++++++++++++++++
>   4 files changed, 55 insertions(+), 5 deletions(-)
>   create mode 100644 test/CodeGen/BPF/xadd_legal.ll
> 
> diff --git a/lib/Target/BPF/BPFInstrInfo.td b/lib/Target/BPF/BPFInstrInfo.td
> index 36724b8..c44702a 100644
> --- a/lib/Target/BPF/BPFInstrInfo.td
> +++ b/lib/Target/BPF/BPFInstrInfo.td
> @@ -614,11 +614,31 @@ class XADD<BPFWidthModifer SizeOp, string OpcodeStr, PatFrag OpNode>
>     let BPFClass = BPF_STX;
>   }
>   
> +class XADD32<BPFWidthModifer SizeOp, string OpcodeStr, PatFrag OpNode>
> +    : TYPE_LD_ST<BPF_XADD.Value, SizeOp.Value,
> +                 (outs GPR32:$dst),
> +                 (ins MEMri:$addr, GPR32:$val),
> +                 "lock *("#OpcodeStr#" *)($addr) += $val",
> +                 [(set GPR32:$dst, (OpNode ADDRri:$addr, GPR32:$val))]> {
> +  bits<4> dst;
> +  bits<20> addr;
> +
> +  let Inst{51-48} = addr{19-16}; // base reg
> +  let Inst{55-52} = dst;
> +  let Inst{47-32} = addr{15-0}; // offset
> +  let BPFClass = BPF_STX;
> +}
> +
>   let Constraints = "$dst = $val" in {
> -def XADD32 : XADD<BPF_W, "u32", atomic_load_add_32>;
> -def XADD64 : XADD<BPF_DW, "u64", atomic_load_add_64>;
> -// undefined def XADD16 : XADD<1, "xadd16", atomic_load_add_16>;
> -// undefined def XADD8  : XADD<2, "xadd8", atomic_load_add_8>;
> +  let Predicates = [BPFNoALU32] in {
> +    def XADDW : XADD<BPF_W, "u32", atomic_load_add_32>;
> +  }
> +
> +  let Predicates = [BPFHasALU32], DecoderNamespace = "BPFALU32" in {
> +    def XADDW32 : XADD32<BPF_W, "u32", atomic_load_add_32>;
> +  }
> +
> +  def XADDD : XADD<BPF_DW, "u64", atomic_load_add_64>;

The XADD64 is changed to XADDD. I guess I am okay with that since
it is consistent with LDD/STD.

>   }
>   
>   // bswap16, bswap32, bswap64
> diff --git a/lib/Target/BPF/BPFMIChecking.cpp b/lib/Target/BPF/BPFMIChecking.cpp
> index 7a4ba96..938e539 100644
> --- a/lib/Target/BPF/BPFMIChecking.cpp
> +++ b/lib/Target/BPF/BPFMIChecking.cpp
> @@ -165,7 +165,9 @@ static bool hasLivingDefs(const MachineInstr &MI,
>   void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
>     for (MachineBasicBlock &MBB : *MF) {
>       for (MachineInstr &MI : MBB) {
> -      if (MI.getOpcode() != BPF::XADD32 && MI.getOpcode() != BPF::XADD64)
> +      if (MI.getOpcode() != BPF::XADDW &&
> +          MI.getOpcode() != BPF::XADDD &&
> +          MI.getOpcode() != BPF::XADDW32)
>           continue;
>   
>         LLVM_DEBUG(MI.dump());
> diff --git a/test/CodeGen/BPF/xadd.ll b/test/CodeGen/BPF/xadd.ll
> index c5f2620..49f0c10 100644
> --- a/test/CodeGen/BPF/xadd.ll
> +++ b/test/CodeGen/BPF/xadd.ll
> @@ -1,5 +1,7 @@
>   ; RUN: not llc -march=bpfel < %s 2>&1 | FileCheck %s
>   ; RUN: not llc -march=bpfeb < %s 2>&1 | FileCheck %s
> +; RUN: not llc -march=bpfel -mattr=+alu32 < %s 2>&1 | FileCheck %s
> +; RUN: not llc -march=bpfeb -mattr=+alu32 < %s 2>&1 | FileCheck %s
>   
>   ; This file is generated with the source command and source
>   ; $ clang -target bpf -O2 -g -S -emit-llvm t.c
> diff --git a/test/CodeGen/BPF/xadd_legal.ll b/test/CodeGen/BPF/xadd_legal.ll
> new file mode 100644
> index 0000000..0d30084
> --- /dev/null
> +++ b/test/CodeGen/BPF/xadd_legal.ll
> @@ -0,0 +1,26 @@
> +; RUN: llc -march=bpfel < %s 2>&1 | FileCheck --check-prefix=CHECK-64 %s
> +; RUN: llc -march=bpfeb < %s 2>&1 | FileCheck --check-prefix=CHECK-64 %s
> +; RUN: llc -march=bpfel -mattr=+alu32 < %s 2>&1 | FileCheck --check-prefix=CHECK-32 %s
> +; RUN: llc -march=bpfeb -mattr=+alu32 < %s 2>&1 | FileCheck --check-prefix=CHECK-32 %s
> +
> +; This file is generated with the source command and source
> +; $ clang -target bpf -O2 -S -emit-llvm t.c
> +; $ cat t.c
> +; int test(int *ptr, unsigned long long a) {
> +;    __sync_fetch_and_add(ptr, a);
> +;    return *ptr;
> +; }
> +;
> +; NOTE: passing unsigned long long as the second operand of __sync_fetch_and_add
> +; could effectively create sub-register reference coming from indexing a full
> +; register which could then exerceise hasLivingDefs inside BPFMIChecker.cpp.
> +
> +define dso_local i32 @test(i32* nocapture %ptr, i64 %a) {
> +entry:
> +  %conv = trunc i64 %a to i32
> +  %0 = atomicrmw add i32* %ptr, i32 %conv seq_cst
> +; CHECK-64: lock *(u32 *)(r1 + 0) += r2
> +; CHECK-32: lock *(u32 *)(r1 + 0) += w2
> +  %1 = load i32, i32* %ptr, align 4
> +  ret i32 %1
> +}
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH LLVM 3/3] bpf: disassembler support for XADD under sub-register mode
  2019-02-27 20:50 ` [PATCH LLVM 3/3] bpf: disassembler support for XADD under sub-register mode Jiong Wang
@ 2019-02-28  6:54   ` Yonghong Song
  2019-02-28  8:02     ` Jiong Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Yonghong Song @ 2019-02-28  6:54 UTC (permalink / raw)
  To: Jiong Wang, alexei.starovoitov; +Cc: daniel, bpf, oss-drivers



On 2/27/19 12:50 PM, Jiong Wang wrote:
> Like the other load/store instructions, "w" register is prefered when
> disassembling BPF_STX | BPF_W | BPF_XADD under sub-register mode.
> 
> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
> ---
>   lib/Target/BPF/Disassembler/BPFDisassembler.cpp | 3 ++-
>   test/MC/BPF/load-store-32.s                     | 3 +++
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/Target/BPF/Disassembler/BPFDisassembler.cpp b/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
> index ed09e44..c5be7cb 100644
> --- a/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
> +++ b/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
> @@ -171,9 +171,10 @@ DecodeStatus BPFDisassembler::getInstruction(MCInst &Instr, uint64_t &Size,
>     if (Result == MCDisassembler::Fail) return MCDisassembler::Fail;
>   
>     uint8_t InstClass = getInstClass(Insn);
> +  uint8_t InstMode = getInstMode(Insn);
>     if ((InstClass == BPF_LDX || InstClass == BPF_STX) &&
>         getInstSize(Insn) != BPF_DW &&
> -      getInstMode(Insn) == BPF_MEM &&
> +      (InstMode == BPF_MEM || InstMode == BPF_XADD) &&
>         STI.getFeatureBits()[BPF::ALU32])
>       Result = decodeInstruction(DecoderTableBPFALU3264, Instr, Insn, Address,

This change triggered the unit test MC/BPF/insn-unit.s failure.

Exit Code: 1

Command Output (stderr):
--
/data/users/yhs/work/llvm/test/MC/BPF/insn-unit.s:60:11: error: CHECK: 
expected string not found in input
// CHECK: c3 92 10 00 00 00 00 00 lock *(u32 *)(r2 + 16) += r9
           ^
<stdin>:25:2: note: scanning from here
  22: c3 92 10 00 00 00 00 00 lock *(u32 *)(r2 + 16) += w9
  ^
<stdin>:25:6: note: possible intended match here
  22: c3 92 10 00 00 00 00 00 lock *(u32 *)(r2 + 16) += w9
      ^
probably objdump -mattr=+alu32 decoding issue.

>                                  this, STI);
> diff --git a/test/MC/BPF/load-store-32.s b/test/MC/BPF/load-store-32.s
> index 73ed9fc..be576d2 100644
> --- a/test/MC/BPF/load-store-32.s
> +++ b/test/MC/BPF/load-store-32.s
> @@ -17,9 +17,12 @@
>     *(u8 *)(r0 + 0) = w7    // BPF_STX | BPF_B
>     *(u16 *)(r1 + 8) = w8   // BPF_STX | BPF_H
>     *(u32 *)(r2 + 16) = w9  // BPF_STX | BPF_W
> +  lock *(u32 *)(r2 + 16) += w9  // BPF_STX | BPF_W | BPF_XADD
>   // CHECK-32: 73 70 00 00 00 00 00 00 	*(u8 *)(r0 + 0) = w7
>   // CHECK-32: 6b 81 08 00 00 00 00 00 	*(u16 *)(r1 + 8) = w8
>   // CHECK-32: 63 92 10 00 00 00 00 00 	*(u32 *)(r2 + 16) = w9
> +// CHECK-32: c3 92 10 00 00 00 00 00 	lock *(u32 *)(r2 + 16) += w9
>   // CHECK: 73 70 00 00 00 00 00 00 	*(u8 *)(r0 + 0) = r7
>   // CHECK: 6b 81 08 00 00 00 00 00 	*(u16 *)(r1 + 8) = r8
>   // CHECK: 63 92 10 00 00 00 00 00 	*(u32 *)(r2 + 16) = r9
> +// CHECK: c3 92 10 00 00 00 00 00 	lock *(u32 *)(r2 + 16) += r9
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH LLVM 3/3] bpf: disassembler support for XADD under sub-register mode
  2019-02-28  6:54   ` Yonghong Song
@ 2019-02-28  8:02     ` Jiong Wang
  0 siblings, 0 replies; 9+ messages in thread
From: Jiong Wang @ 2019-02-28  8:02 UTC (permalink / raw)
  To: Yonghong Song; +Cc: alexei.starovoitov, daniel, bpf, oss-drivers

On Thu, Feb 28, 2019 at 6:54 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 2/27/19 12:50 PM, Jiong Wang wrote:
> > Like the other load/store instructions, "w" register is prefered when
> > disassembling BPF_STX | BPF_W | BPF_XADD under sub-register mode.
> >
> > Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
> > ---
> >   lib/Target/BPF/Disassembler/BPFDisassembler.cpp | 3 ++-
> >   test/MC/BPF/load-store-32.s                     | 3 +++
> >   2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/Target/BPF/Disassembler/BPFDisassembler.cpp b/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
> > index ed09e44..c5be7cb 100644
> > --- a/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
> > +++ b/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
> > @@ -171,9 +171,10 @@ DecodeStatus BPFDisassembler::getInstruction(MCInst &Instr, uint64_t &Size,
> >     if (Result == MCDisassembler::Fail) return MCDisassembler::Fail;
> >
> >     uint8_t InstClass = getInstClass(Insn);
> > +  uint8_t InstMode = getInstMode(Insn);
> >     if ((InstClass == BPF_LDX || InstClass == BPF_STX) &&
> >         getInstSize(Insn) != BPF_DW &&
> > -      getInstMode(Insn) == BPF_MEM &&
> > +      (InstMode == BPF_MEM || InstMode == BPF_XADD) &&
> >         STI.getFeatureBits()[BPF::ALU32])
> >       Result = decodeInstruction(DecoderTableBPFALU3264, Instr, Insn, Address,
>
> This change triggered the unit test MC/BPF/insn-unit.s failure.
>
> Exit Code: 1
>
> Command Output (stderr):
> --
> /data/users/yhs/work/llvm/test/MC/BPF/insn-unit.s:60:11: error: CHECK:
> expected string not found in input
> // CHECK: c3 92 10 00 00 00 00 00 lock *(u32 *)(r2 + 16) += r9
>            ^
> <stdin>:25:2: note: scanning from here
>   22: c3 92 10 00 00 00 00 00 lock *(u32 *)(r2 + 16) += w9
>   ^
> <stdin>:25:6: note: possible intended match here
>   22: c3 92 10 00 00 00 00 00 lock *(u32 *)(r2 + 16) += w9
>       ^
> probably objdump -mattr=+alu32 decoding issue.

Ah, apology. The disassembler change was added at last minute and I was
too confident with the trival change that forgetting to run full regression.

Yes, the issue is the BPF_W insn will show in "w" register format.
So, need the following minior change on the testcase:

diff --git a/test/MC/BPF/insn-unit.s b/test/MC/BPF/insn-unit.s
index c61c002..ff56cfa 100644
--- a/test/MC/BPF/insn-unit.s
+++ b/test/MC/BPF/insn-unit.s
@@ -57,7 +57,8 @@

   lock *(u32 *)(r2 + 16) += r9  // BPF_STX | BPF_W | BPF_XADD
   lock *(u64 *)(r3 - 30) += r10 // BPF_STX | BPF_DW | BPF_XADD
-// CHECK: c3 92 10 00 00 00 00 00      lock *(u32 *)(r2 + 16) += r9
+// CHECK-64: c3 92 10 00 00 00 00 00   lock *(u32 *)(r2 + 16) += r9
+// CHECK-32: c3 92 10 00 00 00 00 00   lock *(u32 *)(r2 + 16) += w9
 // CHECK: db a3 e2 ff 00 00 00 00      lock *(u64 *)(r3 - 30) += r10

 // ======== BPF_JMP Class ========

Regards,
Jiong

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD
  2019-02-28  6:46   ` Yonghong Song
@ 2019-02-28  8:20     ` Jiong Wang
  0 siblings, 0 replies; 9+ messages in thread
From: Jiong Wang @ 2019-02-28  8:20 UTC (permalink / raw)
  To: Yonghong Song; +Cc: alexei.starovoitov, daniel, bpf, oss-drivers

On Thu, Feb 28, 2019 at 6:47 AM Yonghong Song <yhs@fb.com> wrote:
<snip>
> > +  // Otherwise, return true if any aliased SuperReg of GPR32 is not dead.
> > +  std::vector<unsigned>::iterator search_begin = GPR64DeadDefs.begin();
> > +  std::vector<unsigned>::iterator search_end = GPR64DeadDefs.end();
> > +  for (auto I : GPR32LiveDefs) {
> > +    bool ActuallyDead = true;
> > +
> > +    for (MCSuperRegIterator SR(I, TRI); SR.isValid(); ++SR) {
> > +       if (std::find(search_begin, search_end, *SR) == search_end) {
>
> maybe you can just return true here?

Good catch, yeah, simpler, will fix in v2.

Regards,
Jiong

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-02-28  8:20 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 20:50 [PATCH LLVM 0/3] bpf: improvements on XADD semantics check and code-gen Jiong Wang
2019-02-27 20:50 ` [PATCH LLVM 1/3] bpf: improve dead Defs check for XADD Jiong Wang
2019-02-28  6:46   ` Yonghong Song
2019-02-28  8:20     ` Jiong Wang
2019-02-27 20:50 ` [PATCH LLVM 2/3] bpf: enable sub-register code-gen " Jiong Wang
2019-02-28  6:51   ` Yonghong Song
2019-02-27 20:50 ` [PATCH LLVM 3/3] bpf: disassembler support for XADD under sub-register mode Jiong Wang
2019-02-28  6:54   ` Yonghong Song
2019-02-28  8:02     ` Jiong Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.