[PATCH] D116441: [AMDGPU][GlobalISel] Select op_sel modifiers for VOP3P

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3382
+  const MachineInstr *MI = MRI.getVRegDef(Src);
+  return (MI && MI->getOpcode() == AMDGPU::G_BITCAST)
+             ? MI->getOperand(1).getReg()
----------------
MI cannot be null


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3409
+  // number of times and in arbitrary order.
+  Register OldSrc = 0;
+  while (OldSrc != Src) {
----------------
Don't need = 0


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3416
+
+    if (MI && MI->getOpcode() == AMDGPU::G_FNEG &&
+        // It's possible to see an f32 fneg here, but unlikely.
----------------
MI cannot be null


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3424
+    // Strip bitcast
+    else if (MI && MI->getOpcode() == AMDGPU::G_BITCAST) {
+      Src = MI->getOperand(1).getReg();
----------------
Ditto


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3427
+    }
+    // Check if <2 x 16s> vector is shuffled and update opsel modifiers.
+    else if (MI && MI->getOpcode() == AMDGPU::G_SHUFFLE_VECTOR) {
----------------
Move comment into else if and fix weird line break


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3441-3448
+    else if (mi_match(Src, MRI,
+                      m_GOr(m_GAnd(m_Reg(LoSrc), m_SpecificICst(0xffff)),
+                            m_GShl(m_Reg(HiSrc), m_SpecificICst(16))))) {
+      bool NegLo =
+          mi_match(LoSrc, MRI, m_GAnyExt(m_GFNeg(m_GTrunc(m_Reg(LoSrc)))));
+      bool LoIsShifted = isShiftHiToLo(MRI, LoSrc, LoSrc);
+      LoSrc = stripBitcast(MRI, LoSrc);
----------------
This matching is a bit heavy. Seems like we're missing some combines?

This at least could use some comments for the pattern being matched


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3458-3459
+      } else {
+        bool NegHi =
+            mi_match(HiSrc, MRI, m_GAnyExt(m_GFNeg(m_GTrunc(m_Reg(HiSrc)))));
+        bool HiIsShifted = isShiftHiToLo(MRI, HiSrc, HiSrc);
----------------
Why would this pattern appear?


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

https://reviews.llvm.org/D116441