[PATCH] D126989: [AMDGPU] gfx11 VOPC instructions

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash marked 17 inline comments as done.
Joe_Nash added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1032
+
+  let Inst{8 - 0} = 0xfa;
+
----------------
foad wrote:
> dp wrote:
> > Most of the code has no spaces around '-' when specifying bit ranges, but the code here and below is different. We should follow the style of existing code. https://llvm.org/docs/CodingStandards.html#golden-rule.
> Or we should switch to `Inst{8...0}` syntax. https://llvm.org/docs/TableGen/ProgRef.html says "The use of hyphen as the range punctuation is deprecated."
I will make it uniform with existing code. It seems like the whole Target could be converted to the new version with a text substitution in the future.


================
Comment at: llvm/test/CodeGen/AMDGPU/move-load-addr-to-valu.mir:22
   ; GCN:   [[REG_SEQUENCE:%[0-9]+]]:vreg_64 = REG_SEQUENCE [[V_AND_B32_e64_]], %subreg.sub0, [[V_AND_B32_e64_1]], %subreg.sub1
-  ; GCN:   [[V_CMP_NE_U64_e64_:%[0-9]+]]:sreg_64_xexec = V_CMP_NE_U64_e64 [[REG_SEQUENCE]], 0, implicit $exec
+  ; GCN:   [[V_CMP_NE_U64_e64_:%[0-9]+]]:sreg_64_xexec = V_CMP_NE_U64_e64 0, [[REG_SEQUENCE]], implicit $exec
   ; GCN:   [[COPY3:%[0-9]+]]:vreg_64 = COPY [[REG_SEQUENCE]], implicit $exec
----------------
rampitec wrote:
> foad wrote:
> > Joe_Nash wrote:
> > > rampitec wrote:
> > > > Why did it change? This is unexpected.
> > > These are the only changes to the tablegen record.
> > > field bit VOPC = 0; -> field bit VOPC = 1;
> > > bits<64> TSFlags (VOPC bit is now set)
> > > 
> > > There is no difference in the SIFixSGPRCopies output. It seems to me as if when the instruction is being constructed it is now commuted. Since this instruction should commute, do you think this is a bad change? 
> > > 
> > V_CMP_NE_U64_e64 is now marked as both VOPC and VOP3, which means that SIInstrInfo::legalizeOperands now sends it to legalizeOperandsVOP2 instead of legalizeOperandsVOP3. legalizeOperandsVOP2 commutes the operands.
> > 
> > This all feels slightly wrong. VOPC is an encoding, and V_CMP_NE_U64_e64 uses the VOP3 encoding not the VOPC encoding, so it should not be marked as VOPC - should it??
> Interesting. This sounds like it needs a revision of every single place we are using isVOPC()...
I will remove let VOPC = 1 from the 64 bit instructions so isVOP3 and isVOPC remain mutually exclusive


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126989