[PATCH] D68092: [AMDGPU] Invert the handling of skip insertion.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:37
+  const SIInstrInfo *TII = nullptr;
+  unsigned SkipThreshold = 0;
+  bool shouldRetainSkip(const MachineBasicBlock &From,
----------------
You could make this a static member and use cl::location with the flag


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:51-53
+  StringRef getPassName() const override {
+    return "SI remove short exec branches";
+  }
----------------
This isn't necessary


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:78-86
+static MachineBasicBlock *getFalseBranch(MachineBasicBlock &SrcMBB,
+                                         MachineBasicBlock *TrueBB) {
+  assert(SrcMBB.succ_size() == 2);
+  MachineBasicBlock::succ_iterator It = SrcMBB.succ_begin();
+  MachineBasicBlock::succ_iterator Next = It;
+  ++Next;
+
----------------
Should use analyzeBranch instead


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:88-89
+
+bool SIRemoveShortExecBranches::shouldRetainSkip(
+    const MachineBasicBlock &From, const MachineBasicBlock &To) const {
+  unsigned NumInstr = 0;
----------------
This function should probably be rewritten at some point, but for now it's probably not important. I would rename it to sound stronger. mustRetainExeczBranch or something?


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:149
+    MachineBasicBlock::iterator I, Next;
+    for (I = MBB.begin(); I != MBB.end(); I = Next) {
+      Next = std::next(I);
----------------
You don't need to scan forward through the whole block to find the branches. You can just check getFirstTerminator (or just call analyzeBranch on the block and check the condition type)


================
Comment at: test/CodeGen/AMDGPU/convergent-inlineasm.ll:7
 ; GCN: v_cmp_ne_u32_e64
-; GCN: ; mask branch
-; GCN: BB{{[0-9]+_[0-9]+}}:
+; GCN: s_cbranch_execz
+; GCN: ; %bb.{{[0-9]+}}:
----------------
I assume the branch was here before and this isn't a change?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68092