Page MenuHomePhabricator

[AMDGPU] table-driven parser/printer for amd_kernel_code_t structure fields
ClosedPublic

Authored by vpykhtin on Feb 11 2016, 9:41 AM.

Details

Summary

This is going to be used in .hsatext disassembler and can be used in current assembler parser (lit tests passed on parsing). Code using this helpers isn't included in this patch.

Benefits:

  • unified approach
  • fast field name lookup on parsing

Later I would like to enhance some of the field naming/syntax using this code.

Diff Detail

Repository
rL LLVM

Event Timeline

vpykhtin updated this revision to Diff 47671.Feb 11 2016, 9:41 AM
vpykhtin retitled this revision from to [AMDGPU] table-driven parser/printer for amd_kernel_code_t structure fields.
vpykhtin updated this object.
vpykhtin added reviewers: tstellarAMD, arsenm.
vpykhtin set the repository for this revision to rL LLVM.
vpykhtin added a subscriber: nhaustov.
arsenm added inline comments.Feb 11 2016, 11:33 AM
lib/Target/AMDGPU/Utils/AMDKernelCodeTUtils.cpp
36

missing line break

40–41

Is there a parser object somewhere this could be kept instead of the static local variable?

52

StringRef should be passed by value

64

Ditto

70

Ditto

94

Const in weird place

122–123

Weird formatting. Body of if and else should be on their own lines

131–135

Ditto

155

const in weird place

arsenm edited edge metadata.Feb 11 2016, 11:35 AM

We currently differ from how the finalizer prints some of these fields (e.g. TRUE for bools, X/XY/XYZ for workitem ids). Are we planning on trying to match that formatting?

We currently differ from how the finalizer prints some of these fields (e.g. TRUE for bools, X/XY/XYZ for workitem ids). Are we planning on trying to match that formatting?

That is exactly I was thinking about.

lib/Target/AMDGPU/Utils/AMDKernelCodeTUtils.cpp
40–41

There is no parsing state and this map has static nature, used only to find field index by String ID. It's only constucted on the first call to get_amd_kernel_code_t_FieldIndex in a lazy fashion.

In general I used different tables for parser and printer as they usually not used together in one binary. get_amd_kernel_code_t_FieldIndex may not be called either.

vpykhtin updated this revision to Diff 47804.EditedFeb 12 2016, 8:00 AM
vpykhtin edited edge metadata.
vpykhtin marked 10 inline comments as done.

per review fixes

Also changed parseBoolField/printBoolField to more generic parseBitField/printBitField as it turned out that PRIVATE_ELEMENT_SIZE field is 2 bit width.

vpykhtin added a project: Restricted Project.Feb 13 2016, 3:41 AM

LGTM with a few nits, mostly LLVM style isn't followed for reference/pointer placement

lib/Target/AMDGPU/Utils/AMDKernelCodeTInfo.h
2

Should have -*- c++ -*- mode marker

lib/Target/AMDGPU/Utils/AMDKernelCodeTUtils.cpp
136

I recently copied the AMD_HSA_BITS_{SET|GET} macros in AMDKernelCodeT you can use here

161

I'm not sure where this is used from, but is a newline needed after this? When this has tests, one that hits this would be good to have

lib/Target/AMDGPU/Utils/AMDKernelCodeTUtils.h
16

This can be replaced with a forward declaration of amd_kernel_code_t

24

LLVM style is & or * to the right

vpykhtin updated this revision to Diff 48199.Feb 17 2016, 9:17 AM
vpykhtin marked 5 inline comments as done.

per review fixes.

lib/Target/AMDGPU/Utils/AMDKernelCodeTUtils.cpp
136

I'm not sure about this macro naming as this has nothing specific to AMD_HSA.

161

its up to caller to add newline

Kind reminder if someone can take a look at this.

arsenm accepted this revision.Mar 1 2016, 4:15 PM
arsenm edited edge metadata.

LGTM

lib/Target/AMDGPU/Utils/AMDKernelCodeTUtils.cpp
26

Capitalize

136

This should use UINT64_C() rather than ull

This revision is now accepted and ready to land.Mar 1 2016, 4:15 PM
This revision was automatically updated to reflect the committed changes.
vpykhtin marked 2 inline comments as done.Mar 6 2016, 5:34 AM

submitted to the trunk with per review items fixed.