[PATCH] D129637: [AMDGPU][MC][GFX11] Correct disassembly of *_e64_dpp opcodes which support op_sel

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:782
+             AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::op_sel) != -1) {
+    insertNamedMCOperand(MI, MCOperand::createImm(getOpSel(MI)),
+                         AMDGPU::OpName::op_sel);
----------------
Petar.Avramovic wrote:
> You don't need to calculate constant to put in opsel operand in disassembler, opsel bits are already set in src_modifiers, inst_printer does not access op_sel operand. You can simply use 0.
> You only need to add op_sel operand so that other two operands dpp8 and fi are at correct index.
> 
> AsmParser uses op_sel operand to parse opsel and then uses that operand to set opsel bit in src_modifiers. Because Srco operands are parsed first, opsel is parsed after all src operands. After opsel bits in src modifiers are set (e.g. after cvtVOP3OpSel or cvtVOP3P) constant value inside opsel operand could be set to 0 since it would not be used any more.
It is technically true setting op_sel operand to 0 will not affect the disassembler output. However, it will put the operands in an inconsistent state with respect to each other, and for instance when you look at this in the debugger it is very confusing. So while it can save some time/complexity, I think its probably worth it to make all operands correct. 


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

https://reviews.llvm.org/D129637