This is an archive of the discontinued LLVM Phabricator instance.

AMDHSA Code Object v3 assembler syntax update
ClosedPublic

Authored by scott.linder on Jun 4 2018, 12:31 PM.

Details

Summary

Update assembler syntax for code object v3 to match https://reviews.llvm.org/D47566

  • Replace/rename most AMDGPU assembler directives/symbols and document them
  • Provide more diagnostics (e.g. for values out of range, missing required values, repeated values, mismatching command-line options)

The new syntax should allow us to remain backwards compatible, even if the underlying descriptor changes.

Diff Detail

Event Timeline

scott.linder created this revision.Jun 4 2018, 12:31 PM
arsenm added inline comments.Jun 4 2018, 12:34 PM
lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
242–245

Can we start omitting entries when they are the default value? One thing that's been bothering me for a while is how much junk always printed out

scott.linder added inline comments.Jun 5 2018, 9:34 AM
lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
242–245

I think that is reasonable, the only caveat being that you might see fewer directives coming out than you explicitly provided (if you specify a default value explicitly). I will enumerate the directives and their default values somewhere and change this to only emit required and non-default values.

Do not output default values when printing .amdhsa_kernel directives

I missed reverting to the original AMDHSAKernelDescriptor.h

kzhuravl added inline comments.Jun 5 2018, 12:24 PM
include/llvm/Support/AMDHSAKernelDescriptor.h
153–154 ↗(On Diff #150022)

Can this be moved to lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h/cpp?

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2649

Yes.

lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
137

My guess would be yes for this one, since we do it in other similar places.

220

probably no, as our current directive *amdgpu_hsa_kernel* does not do that.

lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.h
71

Can STI be made the first argument?

Address feedback

I am still not sure where "quote a string so that MC understands it" is; there is a static function PrintQuotedString in lib/MC/MCAsmStreamer.cpp that should maybe be moved somewhere else? For now I do what we do for other quoted strings (quote but not escape).

Missing codegen tests.

Rebase, update syntax, expand tests

kzhuravl added inline comments.Jun 13 2018, 6:50 PM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2706

I think we should not be emitting this for v3.

Don't emit v2 kernel symbol type

kzhuravl added inline comments.Jun 14 2018, 10:02 AM
docs/AMDGPUUsage.rst
4396

GroupSegmentFixedSize -> GROUP_SEGMENT_FIXED_SIZE

4398

PrivateSegmentFixedSize -> PRIVATE_SEGMENT_FIXED_SIZE

4400

EnableSGPRPrivateSegmentBuffer -> ENABLE_SGPR_PRIVATE_SEGMENT_BUFFER

4402

EnableSGPRDispatchPtr -> ENABLE_SGPR_DISPATCH_PTR

4404

EnableSGPRQueuePtr -> ENABLE_SGPR_QUEUE_PTR

4406

EnableSGPRKernargSegmentPtr -> ENABLE_SGPR_KERNARG_SEGMENT_PTR

4408

EnableSGPRDispatchID -> ENABLE_SGPR_DISPATCH_ID

4410

ditto

4412

ditto

4414

EnableSGPRGridWorkgroupCountX -> ENABLE_SGPR_GRID_WORKGROUP_COUNT_X

It is currently not implemented in the CP and must always be 0. What is the point of making this directive available to the user at this time?

4416

ditto

4418

ditto

4422

Should default to 1. Is it actually ok to provide this directive? amdhsa_system_sgpr_workgroup_id_x is always initialized.

kzhuravl added inline comments.Jun 14 2018, 2:29 PM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2758

sizeof returns number of bytes. isUint (and similar) take in number of bits.

kzhuravl added inline comments.Jun 15 2018, 7:31 AM
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
237

Use CurrentProgramInfo instead.

241–242

Info.NumVGPR -> CurrentProgramInfo.NumVGPRsForWavesPerEU
Info.NumExplicitSGPR -> CurrentProgramInfo. NumSGPRsForWavesPerEU - IsaInfo::getNumExtraSGPRs...
Info.UsesVCC -> CurrentProgramInfo.VCCUsed
Info.UsesFlatScratch -> CurrentProgramInfo.FlatUsed

Otherwise you are not honoring requested waves per eu.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1852

Did you mean .amdgcn.next_free_{v, s}gpr?

1855

Same as above.

2677

Make less than 80 chars.

2800–2813

amdhsa_user_sgpr_grid_workgroup_count_x/y/z should be removed. see https://reviews.llvm.org/D48191

Address feedback

  • Update KD field names in asm directive docs
  • Remove directives which effect ENABLE_SGPR_GRID_WORKGROUP_COUNT_{X,Y,Z}
  • Default .amdhsa_system_sgpr_workgroup_id_x to 1
  • Fix bug with using sizeof in context of bit count
  • Honor default/requested waves per EU in AsmPrinter
  • Fix formatting
This revision is now accepted and ready to land.Jun 21 2018, 11:13 AM
This revision was automatically updated to reflect the committed changes.

Is it so that the updated documentation is valid only if code-object-v3 is specified?
Is the V3 code object compatible with the latest ROCm release?

Is it so that the updated documentation is valid only if code-object-v3 is specified?

Yes

Is the V3 code object compatible with the latest ROCm release?

Not yet

Is it so that the updated documentation is valid only if code-object-v3 is specified?

Yes

Is the V3 code object compatible with the latest ROCm release?

Not yet

So this change could confuse AMDGPU asm programmers.

Is it so that the updated documentation is valid only if code-object-v3 is specified?

Yes

Is the V3 code object compatible with the latest ROCm release?

Not yet

So this change could confuse AMDGPU asm programmers.

https://reviews.llvm.org/D48497