[MC][AMDGPU] Warn when disassembling a v_cmpx with non-exec destination

[gandhi21299]

Summary

Emit a warning the destination of a v_cmpx instruction is not
EXEC, except when the source is an immediate zero operand.

[gandhi21299]

minor nits

[gandhi21299]

removed debug prints

[foad]

This is pretty much the same as D133334 right?

I'm still not too happy about emitting warnings without using proper diagnostics. The reason is that when the disassembler is being used as a library then it's probably not appropriate for it just to write warnings to stderr.

[Joe_Nash]

Since printing to errs() seems off the table, some other potential options:

• Add proper warning infrastructure to the disassembler
• Wrap the check in DEBUG and print to dbgs() ?
• Make it a disassembly failure, but only on a new architecture, so we don't break backwards compatibility

[gandhi21299]

In D147424#4239901, @foad wrote:

This is pretty much the same as D133334 right?

I'm still not too happy about emitting warnings without using proper diagnostics. The reason is that when the disassembler is being used as a library then it's probably not appropriate for it just to write warnings to stderr.

Yes, it's almost the same. I was facing some rebase difficulties so I created a new revision. I am in favor of having a diagnostics infrastructure in disassembler for emitting warnings/errors as well. I will work on it.

[gandhi21299]

In D147424#4240712, @Joe_Nash wrote:

Since printing to errs() seems off the table, some other potential options:

• Add proper warning infrastructure to the disassembler

Done

• Wrap the check in DEBUG and print to dbgs() ?

I am not sure how that helps. I wrapped the warning-emission in LLVM_DEBUG and added -debug, the warning still does not sync with the instruction it corresponds to. Unless I did not understand this option correctly.

[Joe_Nash]

• Wrap the check in DEBUG and print to dbgs() ?

I am not sure how that helps. I wrapped the warning-emission in LLVM_DEBUG and added -debug, the warning still does not sync with the instruction it corresponds to. Unless I did not understand this option correctly.

This would help with the issue raised by Jay "when the disassembler is being used as a library then it's probably not appropriate for it just to write warnings to stderr." The warning being unsynced seems like a separate issue.

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

DecodeStatus AMDGPUDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
   bool IsSOPK = MCII->get(MI.getOpcode()).TSFlags & SIInstrFlags::SOPK;
   if (Res && ImmLitIdx != -1 && !IsSOPK)
     Res = convertFMAanyK(MI, ImmLitIdx);
 
   // 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());
+
+  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() && AMDGPU::isVCMPX64(Desc) && MI.getNumOperands() > 3 &&
+      !OpIsExec(MI.getOperand(1)) && !OpIsZero(MI.getOperand(3))) {
+    errs() << "Warning: Non-exec destination operand\n";
+  }
   return Res;
 }
 
 DecodeStatus AMDGPUDisassembler::convertEXPInst(MCInst &MI) const {
   if (STI.hasFeature(AMDGPU::FeatureGFX11)) {
     // The MCInst still has these fields even though they are no longer encoded
     // in the GFX11 instruction.
     insertNamedMCOperand(MI, MCOperand::createImm(0), AMDGPU::OpName::vm);

llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp

if (AMDGPU::hasNamedOperand(Opcode, op_sel_hi)) {
     if (AMDGPU::hasNamedOperand(Opcode, src1))
       return OP_SEL_HI_2;
     if (AMDGPU::hasNamedOperand(Opcode, src0))
       return OP_SEL_HI_1 | OP_SEL_HI_2;
   }
   return OP_SEL_HI_0 | OP_SEL_HI_1 | OP_SEL_HI_2;
 }
 
-static bool isVCMPX64(const MCInstrDesc &Desc) {
-  return (Desc.TSFlags & SIInstrFlags::VOP3) &&
-         Desc.hasImplicitDefOfPhysReg(AMDGPU::EXEC);
-}
-
 void SIMCCodeEmitter::encodeInstruction(const MCInst &MI, raw_ostream &OS,
                                         SmallVectorImpl<MCFixup> &Fixups,
                                         const MCSubtargetInfo &STI) const {
   int Opcode = MI.getOpcode();
   APInt Encoding, Scratch;
   getBinaryCodeForInstr(MI, Fixups, Encoding, Scratch,  STI);
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
   unsigned bytes = Desc.getSize();
 
   // Set unused op_sel_hi bits to 1 for VOP3P and MAI instructions.
   // Note that accvgpr_read/write are MAI, have src0, but do not use op_sel.
   if ((Desc.TSFlags & SIInstrFlags::VOP3P) ||
       Opcode == AMDGPU::V_ACCVGPR_READ_B32_vi ||
       Opcode == AMDGPU::V_ACCVGPR_WRITE_B32_vi) {
     Encoding |= getImplicitOpSelHiEncoding(Opcode);
   }
 
   // GFX10+ v_cmpx opcodes promoted to VOP3 have implied dst=EXEC.
   // Documentation requires dst to be encoded as EXEC (0x7E),
   // but it looks like the actual value encoded for dst operand
   // is ignored by HW. It was decided to define dst as "do not care"
   // in td files to allow disassembler accept any dst value.
   // However, dst is encoded as EXEC for compatibility with SP3.
-  if (AMDGPU::isGFX10Plus(STI) && isVCMPX64(Desc)) {
+  if (AMDGPU::isGFX10Plus(STI) && AMDGPU::isVCMPX64(Desc)) {
     assert((Encoding & 0xFF) == 0);
     Encoding |= MRI.getEncodingValue(AMDGPU::EXEC_LO);
   }
 
   for (unsigned i = 0; i < bytes; i++) {
     OS.write((uint8_t)Encoding.extractBitsAsZExtValue(8, 8 * i));
   }
 

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

 //===- AMDGPUBaseInfo.h - Top level definitions for AMDGPU ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_AMDGPU_UTILS_AMDGPUBASEINFO_H
 #define LLVM_LIB_TARGET_AMDGPU_UTILS_AMDGPUBASEINFO_H
 
+#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
 #include "SIDefines.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Alignment.h"
 #include <array>
 #include <functional>
 #include <utility>

 }
 
 /// \returns true if the intrinsic is divergent
 bool isIntrinsicSourceOfDivergence(unsigned IntrID);
 
 /// \returns true if the intrinsic is uniform
 bool isIntrinsicAlwaysUniform(unsigned IntrID);
 
+inline bool isVCMPX64(const MCInstrDesc &Desc) {
+  return (Desc.TSFlags & SIInstrFlags::VOP3) &&
+         Desc.hasImplicitDefOfPhysReg(llvm::AMDGPU::EXEC);
+}
+
 } // end namespace AMDGPU
 
 raw_ostream &operator<<(raw_ostream &OS,
                         const AMDGPU::IsaInfo::TargetIDSetting S);
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_AMDGPU_UTILS_AMDGPUBASEINFO_H

llvm/test/MC/Disassembler/AMDGPU/gfx10-vop3cx-errs.txt

+# RUN: llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=+wavefrontsize32,-wavefrontsize64 -disassemble < %s 2>&1 | FileCheck -check-prefix=GFX10 %s
+# RUN: llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 -disassemble < %s 2>&1 | FileCheck -check-prefix=GFX10 %s
+
+# FIXME: Warning should be aligned with the instruction
+# GFX10: Warning: Non-exec destination operand
+
+# GFX10: v_cmpx_f_f64_e64 exec, v[1:2]
+0x7e,0x00,0x30,0xd4,0x7e,0x02,0x02,0x00
+
+# GFX10: v_cmpx_f_f64_e64 s[0:1], v[1:2]
+0x7e,0x00,0x30,0xd4,0x00,0x02,0x02,0x00

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

-# RUN: llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=+wavefrontsize32,-wavefrontsize64 -disassemble -show-encoding < %s | FileCheck -strict-whitespace -check-prefix=GFX10 %s
-# RUN: llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 -disassemble -show-encoding < %s | FileCheck -strict-whitespace -check-prefix=GFX10 %s
+# RUN: llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=+wavefrontsize32,-wavefrontsize64 -disassemble -show-encoding < %s | FileCheck -strict-whitespace -check-prefix=GFX10 --implicit-check-not="Warning:" %s
+# RUN: llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 -disassemble -show-encoding < %s | FileCheck -strict-whitespace -check-prefix=GFX10 --implicit-check-not="Warning:" %s
 
 
 # GFX10: v_cmpx_class_f16_e64 -1, v2             ; encoding: [0x7e,0x00,0x9f,0xd4,0xc1,0x04,0x02,0x00]
 0x7e,0x00,0x9f,0xd4,0xc1,0x04,0x02,0x00
 
 # GFX10: v_cmpx_class_f16_e64 -4.0, v2           ; encoding: [0x7e,0x00,0x9f,0xd4,0xf7,0x04,0x02,0x00]
 0x7e,0x00,0x9f,0xd4,0xf7,0x04,0x02,0x00