[PATCH] D130192: [RISCV] Avoid redundant branch-to-branch when expanding cmpxchg

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp:531
+    return false;
+  while (MBBI != E && MBBI->isDebugInstr())
+    MBBI++;
----------------
I think this is `skipDebugInstructionsForward`?


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp:535
+  // If we have a masked cmpxchg, match AND dst, DestReg, MaskReg.
+  if (IsMasked) {
+    if (MBBI == E || MBBI->getOpcode() != RISCV::AND)
----------------
Can we avoid an `IsMasked` parameter by checking `MaskReg.isValid()`?


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp:540
+    Register ANDOp2 = MBBI->getOperand(2).getReg();
+    if (!((ANDOp1 == DestReg && ANDOp2 == MaskReg) || (ANDOp1 == MaskReg && ANDOp2 == DestReg)))
+      return false;
----------------
80 columns


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp:540
+    Register ANDOp2 = MBBI->getOperand(2).getReg();
+    if (!((ANDOp1 == DestReg && ANDOp2 == MaskReg) || (ANDOp1 == MaskReg && ANDOp2 == DestReg)))
+      return false;
----------------
craig.topper wrote:
> 80 columns
Might be a little better as

```
if (!(ANDOp1 == DestReg && ANDOp2 == MaskReg) &&
    !(ANDOp1 == MaskReg && ANDOp2 == DestReg))
```

but I'm not sure.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp:555
+  Register BNEOp1 = MBBI->getOperand(1).getReg();
+  if (!((BNEOp0 == DestReg && BNEOp1 == CmpValReg) || (BNEOp0 == CmpValReg && BNEOp1  == DestReg)))
+    return false;
----------------
80 columns


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp:555
+  Register BNEOp1 = MBBI->getOperand(1).getReg();
+  if (!((BNEOp0 == DestReg && BNEOp1 == CmpValReg) || (BNEOp0 == CmpValReg && BNEOp1  == DestReg)))
+    return false;
----------------
craig.topper wrote:
> 80 columns
Same Demorgan suggestion from above


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp:586
+  Register NewValReg = MI.getOperand(4).getReg();
+  Register MaskReg = IsMasked ? MI.getOperand(5).getReg() : Register(0);
+
----------------
`Register(0)` should be `Register()`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130192/new/

https://reviews.llvm.org/D130192