[AMDGPU][MC] Warn when disassembling v_cmpx instructions

[gandhi21299]

Summary

Based on the discussions in D130345,
AMDGPUDisassembler should emit warnings when v_cmpx*
instructions are disassembled with a non-exec destination,
except when the source is an immediate zero operand.

[foad]

It seems like target disassemblers in general do not emit warnings, and are lacking the machinery to emit them as proper diagnostics with location information.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

720

Warnings should go to errs() shouldn't they?

llvm/test/MC/Disassembler/AMDGPU/gfx10_vop3cx.txt

1071

Might be better to do this by changing the RUN lines to use FileCheck --implicit-check-not="Warning:", to catch all unexpected warnings.

[kosarev]

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

720

What if to instead say what is expected?

Also, I guess it may be helpful if we emit the instruction itself and maybe its address as well? Might save the user some time and nerves locating the problematic instruction, especially if the warning doesn't go to that same stream we direct disassembled code to.

[dp]

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

717–718

We could reuse isVCMPX64 here. It may be defined in AMDGPUBaseInfo and reused where necessary.

718

Why is MI.getNumOperands() >= 5 important?

719

This code checks operands of decoded MCInst, but decoded instruction does not provide any information about what dst was encoded in the original binary representation. To check the dst value, we have to work with the original binary code.

llvm/test/MC/Disassembler/AMDGPU/gfx10_vop3cx.txt

1071

I agree. Also, I suggest creating a separate file gfx10_vop3cx_errs.txt and add new tests for this feature there. The tests should include regular dst values (0x7e and 0), as well as a couple of values that trigger the warning.

[dp]

The summary should be corrected.

except when the source is an immediate zero operand

We should allow dst=EXEC and dst=0. The latter encoding has been used for a while for historical reasons, and we should not warn in this case.

[gandhi21299]

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

719

Do you mean use Bytes instead?

[dp]

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

719

Yes, we should use Bytes.

[gandhi21299]

I cannot quite parse 64-bit v_cmpx* instructions after reading the ISA docs several times. I need to implement checks for the source and destination operands. Any pointers?

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

if (VDstIn_Idx != -1) {
     }
   }
 
   int ImmLitIdx =
       AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::imm);
   if (Res && ImmLitIdx != -1)
     Res = convertFMAanyK(MI, ImmLitIdx);
 
+  const MCInstrDesc &Desc = MCII->get(MI.getOpcode());
+  auto OpIsZero = [](MCOperand &Op) {
+    return (Op.isImm() && Op.getImm() == 0);
+  };
+  auto OpIsExec = [](MCOperand &Op) {
+    return (Op.isReg() && Op.getReg() == AMDGPU::EXEC);
+  };
+
+  if (isGFX10Plus() && (Desc.TSFlags & SIInstrFlags::VOP3) &&
+      Desc.hasImplicitDefOfPhysReg(AMDGPU::EXEC) && MI.getNumOperands() >= 5 &&
+      !OpIsExec(MI.getOperand(1)) && !OpIsZero(MI.getOperand(3))) {
+    outs() << "Warning: unexpected dst value\n";
+  }
+
   // if the opcode was not recognized we'll assume a Size of 4 bytes
   // (unless there are fewer bytes left)
   Size = Res ? (MaxInstBytesNum - Bytes.size())
              : std::min((size_t)4, Bytes_.size());
   return Res;
 }
 
 DecodeStatus AMDGPUDisassembler::convertEXPInst(MCInst &MI) const {

llvm/test/MC/Disassembler/AMDGPU/gfx10_vop3cx.txt

 0x7e,0x00,0xd8,0xd4,0x7f,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f16_e64 exec_lo, v2            ; encoding: [0x7e,0x00,0xd8,0xd4,0x7e,0x04,0x02,0x00]
 0x7e,0x00,0xd8,0xd4,0x7e,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f16_e64 m0, v2                 ; encoding: [0x7e,0x00,0xd8,0xd4,0x7c,0x04,0x02,0x00]
 0x7e,0x00,0xd8,0xd4,0x7c,0x04,0x02,0x00
 
+# GFX10: Warning: unexpected dst value
 # GFX10: v_cmpx_f_f16_e64 s1, v2                 ; encoding: [0x7e,0x00,0xd8,0xd4,0x01,0x04,0x02,0x00]
 0x7e,0x00,0xd8,0xd4,0x01,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f16_e64 s101, v2               ; encoding: [0x7e,0x00,0xd8,0xd4,0x65,0x04,0x02,0x00]
 0x7e,0x00,0xd8,0xd4,0x65,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f16_e64 v1, -1                 ; encoding: [0x7e,0x00,0xd8,0xd4,0x01,0x83,0x01,0x00]
 0x7e,0x00,0xd8,0xd4,0x01,0x83,0x01,0x00

 0x7e,0x00,0x30,0xd4,0x01,0x05,0x02,0x20
 
 # GFX10: v_cmpx_f_f64_e64 0, v[2:3]              ; encoding: [0x7e,0x00,0x30,0xd4,0x80,0x04,0x02,0x00]
 0x7e,0x00,0x30,0xd4,0x80,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f64_e64 0.5, v[2:3]            ; encoding: [0x7e,0x00,0x30,0xd4,0xf0,0x04,0x02,0x00]
 0x7e,0x00,0x30,0xd4,0xf0,0x04,0x02,0x00
 
+# GFX10-NOT: Warning: unexpected dst value
 # GFX10: v_cmpx_f_f64_e64 exec, v[1:2]           ; encoding: [0x7e,0x00,0x30,0xd4,0x7e,0x02,0x02,0x00]
 0x7e,0x00,0x30,0xd4,0x7e,0x02,0x02,0x00
 
 # GFX10: v_cmpx_f_f64_e64 exec, v[2:3]           ; encoding: [0x7e,0x00,0x30,0xd4,0x7e,0x04,0x02,0x00]
 0x7e,0x00,0x30,0xd4,0x7e,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f64_e64 s[2:3], v[2:3]         ; encoding: [0x7e,0x00,0x30,0xd4,0x02,0x04,0x02,0x00]
 0x7e,0x00,0x30,0xd4,0x02,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f64_e64 s[4:5], v[2:3]         ; encoding: [0x7e,0x00,0x30,0xd4,0x04,0x04,0x02,0x00]
 0x7e,0x00,0x30,0xd4,0x04,0x04,0x02,0x00
 
 # GFX10: v_cmpx_f_f64_e64 v[1:2], -1             ; encoding: [0x7e,0x00,0x30,0xd4,0x01,0x83,0x01,0x00]
 0x7e,0x00,0x30,0xd4,0x01,0x83,0x01,0x00
 
 # GFX10: v_cmpx_f_f64_e64 v[1:2], -4.0           ; encoding: [0x7e,0x00,0x30,0xd4,0x01,0xef,0x01,0x00]
 0x7e,0x00,0x30,0xd4,0x01,0xef,0x01,0x00
 
 # GFX10: v_cmpx_f_f64_e64 v[1:2], -v[2:3]        ; encoding: [0x7e,0x00,0x30,0xd4,0x01,0x05,0x02,0x40]
 0x7e,0x00,0x30,0xd4,0x01,0x05,0x02,0x40
 
+# GFX10: Warning: unexpected dst value
 # GFX10: v_cmpx_f_f64_e64 v[1:2], 0              ; encoding: [0x7e,0x00,0x30,0xd4,0x01,0x01,0x01,0x00]
 0x7e,0x00,0x30,0xd4,0x01,0x01,0x01,0x00
 
 # GFX10: v_cmpx_f_f64_e64 v[1:2], 0.5            ; encoding: [0x7e,0x00,0x30,0xd4,0x01,0xe1,0x01,0x00]
 0x7e,0x00,0x30,0xd4,0x01,0xe1,0x01,0x00
 
 # GFX10: v_cmpx_f_f64_e64 v[1:2], exec           ; encoding: [0x7e,0x00,0x30,0xd4,0x01,0xfd,0x00,0x00]
 0x7e,0x00,0x30,0xd4,0x01,0xfd,0x00,0x00