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

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added a comment.

Looks good, thanks, just left some small comments.



================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:325
+
+// Determine if a register is in use in the range (Stop..BB.end].
+static bool isRegisterInUseAfter(MachineInstr &Stop, MCRegister Reg,
----------------
I think the comment is not completely correct.
The function checks if Reg is in use after `Stop`. If Reg is dead after Stop but still defined and used afterwards, this function will still return false (because Reg is not in use directly after stop).

The fact that the function uses backwards iteration to find the set of live registers can be considered an implementation detail.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:333-335
+  MachineBasicBlock::reverse_iterator A = MBB.rend();
+
+  for (++A; A != Stop; ++A) {
----------------
tsymalla wrote:
> sebastian-ne wrote:
> > I think a reverse iterator iterates from MBB.rbegin() to MBB.rend() (I’m always confused by this, but it’s used in SIOptimizeExecMaskingPreRA.cpp:412 in this way).
> Good catch. rbegin() == end(), while rend() == begin().
I think the first ++A is wrong, rbegin() should already be a valid iterator?


================
Comment at: llvm/test/CodeGen/AMDGPU/vcmp-saveexec-to-vcmpx.ll:168
+declare i64 @llvm.amdgcn.icmp.i32(i32, i32, i32) #0
\ No newline at end of file

----------------
Nit: There’s a newline missing at the end of the file.


================
Comment at: llvm/test/CodeGen/AMDGPU/vcmp-saveexec-to-vcmpx.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+# RUN: llc -march=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 %s -o - | FileCheck -check-prefix=GCN %s
----------------
This test does not look autogenerated?
Can you add a short comment that explains what this test does? (The test and function name contain vcmpx, but the code does not use v_cmpx, which makes it non-obvious.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119696