This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fix illegally constant folding from V_MOV_B32_sdwa
ClosedPublic

Authored by arsenm on May 16 2020, 10:56 AM.

Details

Summary

This was assumed to be a simple move, and interpreting the immediate
modifier operand as a materialized immediate. Apparently the SDWA pass
never produces these, but GlobalISel does emit these for some vector
shuffles.

Diff Detail

Event Timeline

arsenm created this revision.May 16 2020, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2020, 10:56 AM

Technically even SDWA or DPP for can move immediate, aren't they? I.e.:

v_mov_b32_sdwa v0, 1 dst_sel:DWORD dst_unused:UNUSED_PRESERVE src0_sel:DWORD ; encoding: [0xf9,0x02,0x00,0x7e,0x81,0x16,0x86,0x00]

Should not SDWA peephole be fixed instead? Interpretation of modifiers as materialized immediate seems to be just a bug.

Moreover, we seem to have another bug in v_mov_b32:

<stdin>:1:15: error: not a valid operand.
v_mov_b32 v0, -v1

This is a legal form but we do not support it. If we fix that then we probably will have issue with src0_modifiers again, right?

Technically even SDWA or DPP for can move immediate, aren't they? I.e.:

v_mov_b32_sdwa v0, 1 dst_sel:DWORD dst_unused:UNUSED_PRESERVE src0_sel:DWORD ; encoding: [0xf9,0x02,0x00,0x7e,0x81,0x16,0x86,0x00]

Should not SDWA peephole be fixed instead? Interpretation of modifiers as materialized immediate seems to be just a bug.

We shouldn't emit a simple immediate materialize using SDWA in the first place, so it's not worth the complexity of trying to handle it

Moreover, we seem to have another bug in v_mov_b32:

<stdin>:1:15: error: not a valid operand.
v_mov_b32 v0, -v1

This is a legal form but we do not support it. If we fix that then we probably will have issue with src0_modifiers again, right?

I think I saw a note before that v_mov_b32 doesn't actually support a VOP3 encoding, so I'm not sure it's valid. We never emit V_MOV_B32_e64 anyway

Technically even SDWA or DPP for can move immediate, aren't they? I.e.:

v_mov_b32_sdwa v0, 1 dst_sel:DWORD dst_unused:UNUSED_PRESERVE src0_sel:DWORD ; encoding: [0xf9,0x02,0x00,0x7e,0x81,0x16,0x86,0x00]

Should not SDWA peephole be fixed instead? Interpretation of modifiers as materialized immediate seems to be just a bug.

We shouldn't emit a simple immediate materialize using SDWA in the first place, so it's not worth the complexity of trying to handle it

Moreover, we seem to have another bug in v_mov_b32:

<stdin>:1:15: error: not a valid operand.
v_mov_b32 v0, -v1

This is a legal form but we do not support it. If we fix that then we probably will have issue with src0_modifiers again, right?

I think I saw a note before that v_mov_b32 doesn't actually support a VOP3 encoding, so I'm not sure it's valid. We never emit V_MOV_B32_e64 anyway

It looks like it does support VOP3 form. I can encode 'v_mov_b32 v0, 100'.

Technically even SDWA or DPP for can move immediate, aren't they? I.e.:

v_mov_b32_sdwa v0, 1 dst_sel:DWORD dst_unused:UNUSED_PRESERVE src0_sel:DWORD ; encoding: [0xf9,0x02,0x00,0x7e,0x81,0x16,0x86,0x00]

Should not SDWA peephole be fixed instead? Interpretation of modifiers as materialized immediate seems to be just a bug.

We shouldn't emit a simple immediate materialize using SDWA in the first place, so it's not worth the complexity of trying to handle it

Moreover, we seem to have another bug in v_mov_b32:

<stdin>:1:15: error: not a valid operand.
v_mov_b32 v0, -v1

This is a legal form but we do not support it. If we fix that then we probably will have issue with src0_modifiers again, right?

I think I saw a note before that v_mov_b32 doesn't actually support a VOP3 encoding, so I'm not sure it's valid. We never emit V_MOV_B32_e64 anyway

It looks like it does support VOP3 form. I can encode 'v_mov_b32 v0, 100'.

That's still VOP1 with literal

It looks like it does support VOP3 form. I can encode 'v_mov_b32 v0, 100'.

That's still VOP1 with literal

I have to disagree. It has VOP1 and VOP3 encodings.

rampitec accepted this revision.May 18 2020, 11:33 AM

Ugh. I have found that modifiers can be encoded, but do not work :( LGTM then.

This revision is now accepted and ready to land.May 18 2020, 11:33 AM