Page MenuHomePhabricator

[AMDGPU][MC] Added support of SCC, VCCZ and EXECZ operands
ClosedPublic

Authored by dp on May 30 2019, 6:43 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

dp created this revision.May 30 2019, 6:43 AM

I think you need to handle these registers in the SIInstrInfo::usesConstantBus() and AMDGPUAsmParser::usesConstantBus()/AMDGPU::isSGPR().

dp added a comment.May 30 2019, 10:43 AM

I may be missing something, but these registers are already handled by AMDGPU::isSGPR() because they are defined in SReg_32RegClass.
The same is true for AMDGPUAsmParser::usesConstantBus.

I can add a check to SIInstrInfo::usesConstantBus() but is not it used by codegen only?
As codegen cannot utilize these registers right now, I do not see any use case and will be unable to add any relevant tests.

In D62660#1523651, @dp wrote:

I may be missing something, but these registers are already handled by AMDGPU::isSGPR() because they are defined in SReg_32RegClass.
The same is true for AMDGPUAsmParser::usesConstantBus.

I can add a check to SIInstrInfo::usesConstantBus() but is not it used by codegen only?
As codegen cannot utilize these registers right now, I do not see any use case and will be unable to add any relevant tests.

Right, isSGPR() uses SReg_32, so this is fine, and AsmParser is also fine. But SIInstrInfo::usesConstantBus() checks for SGPR_32 and SGPR_64, thus missing it. I am not sure if it is possible to write a test, but I would guess an inline asm could be written to violate constant bus limit. I would at least list them there to have no issues if codegen will start using these.

dp updated this revision to Diff 202416.May 31 2019, 4:41 AM

Corrected SIInstrInfo::usesConstantBus

arsenm added inline comments.May 31 2019, 5:56 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3890–3895 ↗(On Diff #202416)

This looks like a separate change

lib/Target/AMDGPU/SIInstrInfo.cpp
2834–2849 ↗(On Diff #202416)

Can we avoid this switch by replacing the AMDGPU::SGPR_32RegClass.contains with SReg_32?

dp marked 4 inline comments as done.May 31 2019, 6:49 AM
dp added inline comments.
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3890–3895 ↗(On Diff #202416)

No, this was previously checked by this code:

if (isInlineValue(RegNo))
  return !isCI() && !isSI() && !isVI();

As isInlineValue has changed I had to correct the check as well

lib/Target/AMDGPU/SIInstrInfo.cpp
2834–2849 ↗(On Diff #202416)

I'll correct this, thanks!

rampitec added inline comments.May 31 2019, 8:42 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
2834–2849 ↗(On Diff #202416)

It should not count implicit use of many registers, we need to be careful here as SReg_* is too broad.

dp updated this revision to Diff 202447.May 31 2019, 9:01 AM
dp marked 2 inline comments as done.

Corrected SIInstrInfo::usesConstantBus

rampitec added inline comments.May 31 2019, 9:15 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
2832 ↗(On Diff #202447)

Implicit exec is also free.

dp marked an inline comment as done.May 31 2019, 9:32 AM
dp added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
2832 ↗(On Diff #202447)

This statement asserts that only M0 and VCC use constant bus when implicit so all other registers are free (including exec). What am I missing here?

rampitec accepted this revision.May 31 2019, 9:50 AM

LGTM

lib/Target/AMDGPU/SIInstrInfo.cpp
2832 ↗(On Diff #202447)

Right, OK.

This revision is now accepted and ready to land.May 31 2019, 9:50 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 6:49 AM