This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Added lds support for MUBUF
ClosedPublic

Authored by dp on Feb 19 2018, 9:53 AM.

Diff Detail

Event Timeline

dp created this revision.Feb 19 2018, 9:53 AM

Why is a separate instruction needed for this? Do these also need to add the implicit m0 use?

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
64–65

This should go in the same header with the other of these InstrMapping function declarations

dp added a comment.Feb 19 2018, 1:32 PM

Why is a separate instruction needed for this? Do these also need to add the implicit m0 use?

I forgot about m0, thanks for pointing that out!

I thought about adding 'lds' modifier to existing instructions but decided against it for the following reasons:

  • lds versions have different semantics and existing patterns are of no use.
  • would need a flag in SIInstrFlags to identify MUBUF instructions that support 'lds'. This would be necessary for parser to check if instruction could use 'lds' and 'tfe'. Also note that there are not many free bits left in SIInstrFlags.
  • would need to add or correct patterns for instructions that support lds to account for additional modifier.
  • would need some validation code in parser to check that 'lds' is supported and consistent with 'tfe'.

Also it is trivial to specify implicit use of m0 for separate instructions. Not sure how to do that conditionally.

It is possible to significantly simplify proposed solution by adding 'lds' modifier to all buffer_load* instructions.
Do you suggest to go that way? Please advise.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
64–65

I did that in the first implementation. Unfortunately the header - SIInstrInfo.h is not included in AsmParser. It has a lot of MachineInstr stuff that do not belong AsmParser. Should we include all that stuff for the sake of one declaration?

In D43472#1012573, @dp wrote:

Why is a separate instruction needed for this? Do these also need to add the implicit m0 use?

I forgot about m0, thanks for pointing that out!

I thought about adding 'lds' modifier to existing instructions but decided against it for the following reasons:

  • lds versions have different semantics and existing patterns are of no use.
  • would need a flag in SIInstrFlags to identify MUBUF instructions that support 'lds'. This would be necessary for parser to check if instruction could use 'lds' and 'tfe'. Also note that there are not many free bits left in SIInstrFlags.
  • would need to add or correct patterns for instructions that support lds to account for additional modifier.
  • would need some validation code in parser to check that 'lds' is supported and consistent with 'tfe'.

Also it is trivial to specify implicit use of m0 for separate instructions. Not sure how to do that conditionally.

We do stuff like Uses = !if(Foo, [EXEC], [VCC, EXEC]) other places

It is possible to significantly simplify proposed solution by adding 'lds' modifier to all buffer_load* instructions.
Do you suggest to go that way? Please advise.

I'm not sure. Separate instructions is probably better since it avoids needing to manually deal with the m0 constraint so is probably less error prone.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
64–65

We have some in AMDGPUBaseInfo.h (e.g. getNamedOperandIdx)

dp updated this revision to Diff 135018.Feb 20 2018, 1:20 AM
dp marked 3 inline comments as done.

Corrected issues found by Matt:

  • added implicit use of M0 for lds opcodes;
  • moved declaration of getMUBUFNoLdsInst to SIInstrInfo.h
artem.tamazov accepted this revision.Feb 20 2018, 4:27 AM

LGTM. I recommend adding an erratic case with both "lds" and "tfe" to the tests.

This revision is now accepted and ready to land.Feb 20 2018, 4:27 AM
dp added a comment.Feb 20 2018, 7:31 AM

LGTM. I recommend adding an erratic case with both "lds" and "tfe" to the tests.

Added:

buffer_load_sbyte v5, off, s[8:11], s3 lds tfe
// NOSICIVI: error: invalid operand for instruction

buffer_load_dword v5, off, s[8:11], s3 tfe lds
// NOSICIVI: error: invalid operand for instruction

In D43472#1013187, @dp wrote:

LGTM. I recommend adding an erratic case with both "lds" and "tfe" to the tests.

Added...

Great, thanks!

This revision was automatically updated to reflect the committed changes.