[PATCH] D52677: [AMDGPU] Match v_swap_b32

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I can see why you would put it in SIShrinkInstructions, but this isn't really a shrinking problem. Can you add a comment that this is really just a generic post-RA peephole that should probably be in a separate pass?



================
Comment at: lib/Target/AMDGPU/SIInstrInfo.h:219
+  for (const MachineOperand &MO : R) {
+    if (MO.isReg()) {
+      if (TargetRegisterInfo::isPhysicalRegister(Reg) &&
----------------
!isReg() and continue to reduce indentation


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.h:226-227
+                 TargetRegisterInfo::isVirtualRegister(Reg)) {
+        if ((TRI.getSubRegIndexLaneMask(SubReg) &
+             TRI.getSubRegIndexLaneMask(MO.getSubReg())).any())
+          return true;
----------------
I think this is a bit difficult to read with the () and .any() on the temporary. Can you use a variable for the &


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.h:234-238
+static bool instReadsReg(const MachineInstr *MI,
+                         unsigned Reg, unsigned SubReg,
+                         const SIRegisterInfo &TRI) {
+  return instAccessReg(MI->uses(), Reg, SubReg, TRI);
+}
----------------
How is this different from MachineInstr::readsRegister? Does that just to add a subreg operand?


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.h:246-247
+
+static std::pair<unsigned, unsigned>
+getSubRegForIndex(unsigned Reg, unsigned Sub, unsigned I,
+                  const SIRegisterInfo &TRI, const MachineRegisterInfo &MRI) {
----------------
RegSubRegPair instead of std::pair?


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.h:286
+  MachineOperand *YTop = nullptr;
+  for (MachineOperand &Op : MRI.use_operands(T)) {
+    if (Op.getSubReg() != Tsub) {
----------------
use_nodbg_operands?


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.h:401
 
+      if (ST.getGeneration() >= AMDGPUSubtarget::GFX9 &&
+          (MI.getOpcode() == AMDGPU::V_MOV_B32_e32 ||
----------------
ST.hasSwap() or something implemented by checking gfx9-insts


================
Comment at: test/CodeGen/AMDGPU/v_swap_b32.mir:502
+    %1 = COPY %2
+...
----------------
Needs a test where the mov/copy has extra implicit uses / defs. I think this should probably skip them if there are. In particular I'm worried about the super-register copy pattern where there's an implicit def of the super-register on the first copy in the sequence


https://reviews.llvm.org/D52677