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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Seems reasonable overall, but I am a bit worried about testing all the possible corner cases.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3457
+          Mods ^= SISrcMods::OP_SEL_1;
+        Src = MI->getOperand(1).getReg();
+      }
----------------
I don't understand how this can be safe, when you never even look at MI->getOperand(2). Did you mean to test the mask elements for equality, e.g. `ShuffleMask[0] == 1` instead of `ShuffleMask[0] != 0`?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3475
+      // like above.
+      NegLo = NegLo != mi_match(LoSrc, MRI, m_GFNeg(m_Reg(LoSrc)));
+
----------------
This is `NegLo ^= ...`


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3478
+      // Ignore HiSrc if it's undef.
+      if (MRI.getVRegDef(HiSrc)->getOpcode() == AMDGPU::G_IMPLICIT_DEF) {
+        if (NegLo)
----------------
Are there tests for this case? If HiSrc is undef then I am a bit surprised that we would ever match m_GShl(m_Reg(HiSrc), m_SpecificICst(16)) above.

Also, do we need an equivalent case for when LoSrc is undef?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3490
+        HiSrc = stripBitcast(MRI, HiSrc);
+        NegHi = NegHi != mi_match(HiSrc, MRI, m_GFNeg(m_Reg(HiSrc)));
+
----------------
`NegHi ^= ...`


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

https://reviews.llvm.org/D116441