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

Repository
rL LLVM

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
243–246 ↗(On Diff #149827)

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
243–246 ↗(On Diff #149827)

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
2631 ↗(On Diff #150022)

Yes.

lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
137 ↗(On Diff #150022)

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

221 ↗(On Diff #150022)

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

lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.h
71 ↗(On Diff #150022)

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
2708 ↗(On Diff #151010)

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
4367 ↗(On Diff #151363)

GroupSegmentFixedSize -> GROUP_SEGMENT_FIXED_SIZE

4369 ↗(On Diff #151363)

PrivateSegmentFixedSize -> PRIVATE_SEGMENT_FIXED_SIZE

4371 ↗(On Diff #151363)

EnableSGPRPrivateSegmentBuffer -> ENABLE_SGPR_PRIVATE_SEGMENT_BUFFER

4373 ↗(On Diff #151363)

EnableSGPRDispatchPtr -> ENABLE_SGPR_DISPATCH_PTR

4375 ↗(On Diff #151363)

EnableSGPRQueuePtr -> ENABLE_SGPR_QUEUE_PTR

4377 ↗(On Diff #151363)

EnableSGPRKernargSegmentPtr -> ENABLE_SGPR_KERNARG_SEGMENT_PTR

4379 ↗(On Diff #151363)

EnableSGPRDispatchID -> ENABLE_SGPR_DISPATCH_ID

4381 ↗(On Diff #151363)

ditto

4383 ↗(On Diff #151363)

ditto

4385 ↗(On Diff #151363)

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?

4387 ↗(On Diff #151363)

ditto

4389 ↗(On Diff #151363)

ditto

4393 ↗(On Diff #151363)

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
2760 ↗(On Diff #151363)

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 ↗(On Diff #151363)

Use CurrentProgramInfo instead.

243–244 ↗(On Diff #151363)

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
1855 ↗(On Diff #151363)

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

1858 ↗(On Diff #151363)

Same as above.

2679 ↗(On Diff #151363)

Make less than 80 chars.

2802–2815 ↗(On Diff #151363)

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