[PATCH] D119696: [AMDGPU] Improve v_cmpx usage on GFX10.3.

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:303
+    MachineInstr &Origin, std::function<bool(MachineInstr *)> Pred,
+    SmallVector<MCRegister, 1> &NonModifiableRegs,
+    bool DisallowDefBetween = true, unsigned MaxInstructions = 5) {
----------------
Instead of taking a SmallVector, you can take an `ArrayRef<MCRegister>`, then you can also pass `{SaveExecSrc0->getReg()}` without needing a vector.

Also, if you needed a vector here because it’s modified inside the function, it is considered better style to pass a `SmallVectorImpl<MCRegister>`, so that the caller can decide the number for inline allocation.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:310-322
+    bool PredResult = Pred(&*A);
+
+    if (!PredResult) {
+      if (DisallowDefBetween)
+        for (MCRegister Reg : NonModifiableRegs)
+          if (A->modifiesRegister(Reg))
+            return nullptr;
----------------
I think this could be clearer:
```
if ((Pred(&*A))
  return &*A;

if (DisallowDefBetween) {
  // …
}
++CurrentIteration;
```


================
Comment at: llvm/test/CodeGen/AMDGPU/branch-relaxation-gfx10-branch-offset-bug.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -march=amdgcn -mcpu=gfx1030 -verify-machineinstrs -amdgpu-s-branch-bits=7 < %s | FileCheck -enable-var-scope -check-prefixes=GCN,GFX1030 %s
----------------
This file doesn’t look autogenerated, but maybe it makes sense to generate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119696