This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][AsmParser] Refine SMRD offset definitions.
ClosedPublic

Authored by kosarev on Jan 20 2023, 9:09 AM.

Details

Summary
  • Fixes the type of default 8-bit offset operands.
  • Adds a test for optional offsets.

This is effectively an NFC.

Diff Detail

Event Timeline

kosarev created this revision.Jan 20 2023, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 9:09 AM
kosarev requested review of this revision.Jan 20 2023, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 9:09 AM

SP3 doesn't seem to allow these optional offsets and we don't have any tests for them, so this assumes they shouldn't be supported. But happy to rework to add the missing tests if we know otherwise.

arsenm added inline comments.Jan 20 2023, 9:31 AM
llvm/test/MC/AMDGPU/gfx7_asm_err.s
1 ↗(On Diff #490889)

Should also include run lines for gfx6 and 8?

foad added a comment.Jan 23 2023, 3:13 AM

"Forbid optional offsets" is a strange way to describe this. How about "make offset mandatory"?

Does the (LLVM or SP3) assembler allow the offset to be omitted on GFX8/9?

dp added a comment.Jan 24 2023, 2:40 AM

The patch itself looks fine, but it breaks backward compatibility with existing code.
I think if you make offset mandatory for GFX6 and GFX7, the same should be done for other GPUs. But IMO preserving compatibility with existing code is more important.
BTW, have you noticed that SP3 does not allow 32-bit offsets with SMRD? LLVM assembler accepts these, but I do not know if HW can handle it.

In D142231#4076375, @dp wrote:

The patch itself looks fine, but it breaks backward compatibility with existing code.
I think if you make offset mandatory for GFX6 and GFX7, the same should be done for other GPUs. But IMO preserving compatibility with existing code is more important.
BTW, have you noticed that SP3 does not allow 32-bit offsets with SMRD? LLVM assembler accepts these, but I do not know if HW can handle it.

I think that worked for CI only

dp added a comment.Jan 24 2023, 3:04 AM

I think that worked for CI only

Yes, it works for SMRD only, not for SMEM. Not sure about SI.

kosarev updated this revision to Diff 500237.Feb 24 2023, 9:25 AM

Reworked to keep optional offsets.

kosarev retitled this revision from [AMDGPU][AsmParser] Forbid optional SMRD offsets on GFX6/GFX7. to [AMDGPU][AsmParser] Refine SMRD offset definitions..Feb 24 2023, 9:25 AM
kosarev edited the summary of this revision. (Show Details)
dp added inline comments.Mar 24 2023, 2:34 AM
llvm/lib/Target/AMDGPU/SMInstructions.td
759 ↗(On Diff #500237)

I assume making this operand optional or mandatory does not really affect anything, does it?

kosarev added inline comments.Mar 29 2023, 9:22 AM
llvm/lib/Target/AMDGPU/SMInstructions.td
759 ↗(On Diff #500237)

isSMRDLiteralOffset() reads like it can't really be optional.

dp accepted this revision.Mar 29 2023, 9:39 AM

LGTM

This revision is now accepted and ready to land.Mar 29 2023, 9:39 AM
This revision was landed with ongoing or failed builds.Mar 30 2023, 7:30 AM
This revision was automatically updated to reflect the committed changes.
kosarev edited the summary of this revision. (Show Details)Mar 30 2023, 7:30 AM