This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC][GFX11][NFC] Update tests for VOP3P.DPP instructions
AbandonedPublic

Authored by dp on Aug 30 2022, 10:43 AM.

Details

Reviewers
Joe_Nash
foad
Summary

Note that tests for DOT opcodes have not been updated. DPP variants of DOT instructions seem to have an incorrect encoding.

Diff Detail

Unit TestsFailed

Event Timeline

dp created this revision.Aug 30 2022, 10:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2022, 10:43 AM
dp requested review of this revision.Aug 30 2022, 10:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2022, 10:43 AM
Joe_Nash added inline comments.Aug 30 2022, 12:40 PM
llvm/test/MC/AMDGPU/gfx11_asm_vop3p_dpp16.s
8

I agree something is weird here with v_dot2_f32_f16 and v_dot2_f32_bf16, I will try to clarify the desired behavior.

20

I'm not sure where you're getting op_sel_hi=7 for v_fma_mix* instructions. From my reading of the spec and comparison with sp3, all values of op_sel and op_sel_hi are supported. The behavior is non-standard for mix opcodes, but the values should be accepted.
Does your observation have anything to do with dpp? Both dpp and non-dpp version should behave the same and allow all values of op_sel.

dp added inline comments.Aug 31 2022, 4:32 AM
llvm/test/MC/AMDGPU/gfx11_asm_vop3p_dpp16.s
20

I remember that SPG stated the following at some point:

When using DPP with VOP3/VOP3P, the OPSEL must be set such that the low result only uses low inputs, and the high result only uses high inputs.

I always thought that this was a suspicious statement, but see the original tests gfx11_asm_vop3p_dpp8.s below, they stated essentially the same:

// For test purpose only. OP_SEL has to be set to all 0 and OP_SEL_HI has to be set to all 1
llvm/test/MC/AMDGPU/gfx11_asm_vop3p_dpp8.s
13

Here is the original comment.

dp updated this revision to Diff 456924.Aug 31 2022, 4:39 AM

Remove FIXME note regarding use of op_sel for fma_mix instructions. I assume op_sel is legal in this context.

Joe_Nash added inline comments.Aug 31 2022, 7:13 AM
llvm/test/MC/AMDGPU/gfx11_asm_vop3p_dpp16.s
20

Oh I see. In my previous reply I was just looking at the rules for vop3p by mistake. The existing comment was actually correct. There is a special case for vop3p with dpp.

When using DPP with VOP3P, OPSEL has to be set to all 0 and OPSEL_HI has to be set to all 1

And in GCNDPPCombine we actually check those rules. So Codegen is doing the right thing.

Sorry for the noise.

dp added a comment.Aug 31 2022, 12:13 PM

When using DPP with VOP3P, OPSEL has to be set to all 0 and OPSEL_HI has to be set to all 1

Does it mean that DPP variants of fma_mix opcodes are encoded incorrectly? Both llvm and SP3 assemblers do not set op_sel_hi to all 1:

v_fma_mix_f32 v255, v1, v2, v3 dpp8:[7,6,5,4,3,2,1,0] ; encoding: [0xff,0x00,0x20,0xcc,0xe9,0x04,0x0e,0x04,0x01,0x77,0x39,0x05]

encoding: VOP3P.DPP8
  clamp             0
  encoding       0xcc
  fi8            0x01
  lane_sel_0     0x07
  lane_sel_1     0x06
  lane_sel_2     0x05
  lane_sel_3     0x04
  lane_sel_4     0x03
  lane_sel_5     0x02
  lane_sel_6     0x01
  lane_sel_7        0
  neg               0
  neg_hi            0
  op             0x20   v_fma_mix_f32_e64_dpp
  op_sel            0
  op_sel_hi         0
  op_sel_hi_2       0
  src0           0x01
  src1          0x102
  src2          0x103
  vdst           0xff

I'll file a ticket on this issue then. I think we should not update tests till the issue is fixed.

In D132957#3761978, @dp wrote:

When using DPP with VOP3P, OPSEL has to be set to all 0 and OPSEL_HI has to be set to all 1

Does it mean that DPP variants of fma_mix opcodes are encoded incorrectly? Both llvm and SP3 assemblers do not set op_sel_hi to all 1:

It is true that there is no check verifying that desired property in the assembler. Filing an issue makes sense, though adding a check for the property seems low priority.

dp added a comment.Sep 1 2022, 8:34 AM

Added new issue.

dp abandoned this revision.Sep 1 2022, 8:35 AM

Tests will be updated after the encoding issue is fixed.