This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Corrected V_*QSAD* instructions to check that dest register is different than any of the src
ClosedPublic

Authored by dp on Jun 7 2017, 10:52 AM.

Details

Summary

The V_MQSAD_PK_U16_U8, V_QSAD_PK_U16_U8, and V_MQSAD_U32_U8 take more than 1 pass in hardware.
For these three instructions, the destination registers must be different than all sources so that the first pass does not overwrite sources for the following passes.

See Bug 33279: https://bugs.llvm.org//show_bug.cgi?id=33279

Diff Detail

Event Timeline

dp created this revision.Jun 7 2017, 10:52 AM
artem.tamazov accepted this revision.Jun 7 2017, 11:02 AM
This revision is now accepted and ready to land.Jun 7 2017, 11:02 AM
arsenm added a comment.Jun 7 2017, 1:16 PM

I don't think it's the assembler's job to be diagnosing high level issues like this. A warning may be useful, but I don't think this should be a hard error

I don't think it's the assembler's job to be diagnosing high level issues like this. A warning may be useful, but I don't think this should be a hard error

Sounds reasonable, but still depends on the point of view. The issue is closely related to HW implementation and similar to the constant bus constraints we already diagnosing.

I think that generally, if the side effects of an instruction (i.e. the result) are not well-defined, then an error shall be flagged. Ones who really need this kind of stuff could use .byte to emit whatever strange code they want. Alternatively, we can implement an option for asm geeks which disables checks like this.

Perhaps it is worth to move further discussion (if any) to bugzilla.

dp added a comment.Jun 8 2017, 6:33 AM

I'm going to correct this fix to avoid using a switch with a list of affected *QSAD* opcodes.
Artem suggested to use 'Constraints' information from td files to identify opcodes with

`Constraints = "@earlyclobber $vdst"`

In asm code this will look like this:

`if (Desc.getOperandConstraint(DstIdx, MCOI::EARLY_CLOBBER) != -1) { ... check limitations on dst ... }`

Does this look reasonable?

dp added a comment.Jun 8 2017, 6:36 AM

But this will create a dependency on https://reviews.llvm.org/D33783

In D34003#776031, @dp wrote:

I'm going to correct this fix to avoid using a switch with a list of affected *QSAD* opcodes.

`if (Desc.getOperandConstraint(DstIdx, MCOI::EARLY_CLOBBER) != -1) { ... check limitations on dst ... }`

Does this look reasonable?

That is exactly what I have in mind, including dependency.

dp updated this revision to Diff 102860.Jun 16 2017, 12:47 PM

Updated to identify affected opcodes using EARLY_CLOBBER constraint

artem.tamazov accepted this revision.Jun 19 2017, 4:07 AM
This revision was automatically updated to reflect the committed changes.