This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix MC tests for v_fmaak_f16 and v_fmamk_f16
ClosedPublic

Authored by foad on Jun 4 2021, 7:57 AM.

Details

Summary

This looks like a mistake when the tests were committed in r363946.
There were two sets of tests for the f32 variant of these instructions,
instead of one set for f16 and one set for f32.

Diff Detail

Event Timeline

foad created this revision.Jun 4 2021, 7:57 AM
foad requested review of this revision.Jun 4 2021, 7:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 7:57 AM
dp accepted this revision.Jun 4 2021, 9:19 AM

LGTM

This revision is now accepted and ready to land.Jun 4 2021, 9:19 AM
Joe_Nash accepted this revision.Jun 4 2021, 12:08 PM

The tests you changed had 2 byte constants vs the 4 byte ones in the remaining f32 tests. But I don't know if that is significant.
LGTM

foad added a comment.Jun 7 2021, 2:45 AM

The tests you changed had 2 byte constants vs the 4 byte ones in the remaining f32 tests. But I don't know if that is significant.

I assumed that the tests with 2 byte literals were originally supposed to be for the f16 instrucction, and someone just forgot to change the opcodes.

This revision was automatically updated to reflect the committed changes.
dp added a comment.Jun 7 2021, 3:08 AM

The tests you changed had 2 byte constants vs the 4 byte ones in the remaining f32 tests. But I don't know if that is significant.

I assumed that the tests with 2 byte literals were originally supposed to be for the f16 instrucction, and someone just forgot to change the opcodes.

It is significant - assembler expects 16 bit literals for 16 bit operands like u16 or f16 - see https://llvm.org/docs/AMDGPUOperandSyntax.html#type-and-size-conversion.
If I remember correctly, HW ignores high 16 bits of literals specified for 16 bit operands.