This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Enable modifiers on V_MOV_B32
Needs ReviewPublic

Authored by danilomilosevic on Feb 17 2023, 8:05 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 8:05 AM
danilomilosevic requested review of this revision.Feb 17 2023, 8:05 AM

Thanks for taking a look at this issue.

First, I am surprised there are not many more crashes without further modifications to codegen passes to account for the changed number of operands. For example
SIFoldOperand.cpp:693
The number of operands has changed, so I would expect removing the operands to break quite badly.

Actually, I think due to the way the operand lists for _e32 instructions are constructed, the modifiers are not taking effect.
If you dump the tablegen records, if the change was effective there should be a src0_modifiers operand on the V_MOV_B32_e32 instruction.

That said, I'm not convinced doing this properly would be worth the effort. Perhaps I should just close the issue.
@arsenm
@foad

llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_vop3_dpp8_from_vop1.txt
435

It's curious that this results in sext(v2). It should print -v2. sext is not supported on v_mov_b32. Perhaps this is caused by the wrong operand class being set, or due to a bug in the inst printer.

Actually, I think due to the way the operand lists for _e32 instructions are constructed, the modifiers are not taking effect.
If you dump the tablegen records, if the change was effective there should be a src0_modifiers operand on the V_MOV_B32_e32 instruction.

I'd expect it to only be on v_mov_b32_e64?

Actually, I think due to the way the operand lists for _e32 instructions are constructed, the modifiers are not taking effect.
If you dump the tablegen records, if the change was effective there should be a src0_modifiers operand on the V_MOV_B32_e32 instruction.

I'd expect it to only be on v_mov_b32_e64?

I guess the spec was changed between gfx9 and gfx10. On gfx10 and newer the docs say modifiers are supported on VOP1 as well as VOP3. But in terms of what our backend would make with just the above patch, it seems the modifiers would be on v_mov_b32_e64 but not v_mov_b32_e32. Hence my ask to check the tablegen records.

foad added a comment.Feb 21 2023, 12:57 AM

Actually, I think due to the way the operand lists for _e32 instructions are constructed, the modifiers are not taking effect.
If you dump the tablegen records, if the change was effective there should be a src0_modifiers operand on the V_MOV_B32_e32 instruction.

I'd expect it to only be on v_mov_b32_e64?

I guess the spec was changed between gfx9 and gfx10. On gfx10 and newer the docs say modifiers are supported on VOP1 as well as VOP3. But in terms of what our backend would make with just the above patch, it seems the modifiers would be on v_mov_b32_e64 but not v_mov_b32_e32. Hence my ask to check the tablegen records.

There are no modifiers in VOP1 - there are no bits in the encoding for them.

  • Fixed sext appearing in v_mod_b32_e64_dpp
  • Fixed disassembler tests

Actually, I think due to the way the operand lists for _e32 instructions are constructed, the modifiers are not taking effect.
If you dump the tablegen records, if the change was effective there should be a src0_modifiers operand on the V_MOV_B32_e32 instruction.

I'd expect it to only be on v_mov_b32_e64?

I guess the spec was changed between gfx9 and gfx10. On gfx10 and newer the docs say modifiers are supported on VOP1 as well as VOP3. But in terms of what our backend would make with just the above patch, it seems the modifiers would be on v_mov_b32_e64 but not v_mov_b32_e32. Hence my ask to check the tablegen records.

There are no modifiers in VOP1 - there are no bits in the encoding for them.

Ah yes, that makes a lot more sense. Sorry, I was not thinking clearly there.
The intention of this patch seems ok then to allow the modifiers on v_mov_b32_e64. Since modifiers are not supported on gfx9 and earlier, it would likely need the introduction of a new pseudo instruction for v_mov_b32, one for gfx10Plus that allows modifiers on VOP3.

But we make limited use of v_mov_b32_e64. So should more work be done to make use of it?
The potential utility is to do something like v_mov_b32 v0, -v1. But in a lot of cases this could be folded into the use of v0 by setting the neg modifier there.