This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Support disassembly for AMDGPU kernel descriptors
ClosedPublic

Authored by rochauha on May 28 2020, 5:20 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rochauha updated this revision to Diff 272666.Jun 23 2020, 3:44 AM
rochauha marked an inline comment as done.

Minor change to the comment about Size.

rochauha marked an inline comment as done.Jun 23 2020, 3:46 AM
rochauha marked 2 inline comments as done.

No idea about the functional logic of this code, but I do wonder whether you'd be better off dividing it into smaller patches for testability.

This patch is for entirely disassembling the kernel descriptor symbol. It happens to be so that many directives affect the kernel descriptor. I will also be adding a new test in this patch.

The problem is that you're trying to do it all at once. There's no need to implement the full disassembly at once. You can do it in a series of patches, one after the other that build towards the primary goal. For example, there are many if (<X is some value>) type cases, which you could omit - just assume those are all false, in earlier versions, and add them in (with corresponding testing) in a later patch. Similarly, you could assume that all results of x & y return some specific value, e.g. 0, and just print that for now. Yes, that means you won't support everything from the point at which this patch lands, but it will make each individual patch easier to reason with. This fits much better with LLVM's preferred approach - please see https://llvm.org/docs/DeveloperPolicy.html#incremental-development.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1274–1277

I don't see a response to this suggestion.

1335

next -> Next

1481

/*IsLittleEndian =*/true -> /*IsLittleEndian=*/true

(and same for AddressSize)

as already asked for once...

1671

Nit: missing full stop.

1673–1674

Put this comment above the if it is referring to, like the exisitng bit of comment.

jhenderson added a comment.EditedJun 24 2020, 2:05 AM

Oh, you could also put the additional behaviour behind a switch in llvm-objdump. That'll mean you won't break all the existing tests too. Once you've finished the development, you could either remove the switch entirely or just change its default (which would allow users to disable the behaviour if they want the less verbose version of the output).

rochauha updated this revision to Diff 273682.Jun 26 2020, 5:11 AM
  • Updated to use Cursor. Now there isn't any neet to maintain CurrentIndex.
  • Other small changes related to coding conventions.

The problem is that you're trying to do it all at once. There's no need to implement the full disassembly at once. You can do it in a series of patches, one after the other that build towards the primary goal. For example, there are many if (<X is some value>) type cases, which you could omit - just assume those are all false, in earlier versions, and add them in (with corresponding testing) in a later patch. Similarly, you could assume that all results of x & y return some specific value, e.g. 0, and just print that for now. Yes, that means you won't support everything from the point at which this patch lands, but it will make each individual patch easier to reason with. This fits much better with LLVM's preferred approach - please see https://llvm.org/docs/DeveloperPolicy.html#incremental-development.

Oh, you could also put the additional behaviour behind a switch in llvm-objdump. That'll mean you won't break all the existing tests too. Once you've finished the development, you could either remove the switch entirely or just change its default (which would allow users to disable the behaviour if they want the less verbose version of the output).

Thanks for the feedback! I'll keep these points in mind :)

Regarding this patch - I think that because it has been reviewed to quite some extent, it doesn't need to be broken into smaller patches now?

rochauha marked 5 inline comments as done.Jun 26 2020, 5:19 AM

Following tests fail:

LLVM :: CodeGen/AMDGPU/call-encoding.ll
LLVM :: CodeGen/AMDGPU/s_code_end.ll
LLVM :: MC/AMDGPU/branch-comment.s
LLVM :: MC/AMDGPU/data.s
LLVM :: MC/AMDGPU/labels-branch-gfx9.s
LLVM :: MC/AMDGPU/labels-branch.s
LLVM :: MC/AMDGPU/offsetbug_once.s
LLVM :: MC/AMDGPU/offsetbug_one_and_one.s
LLVM :: MC/AMDGPU/offsetbug_twice.s
LLVM :: MC/AMDGPU/s_endpgm.s
LLVM :: Object/AMDGPU/objdump.s
LLVM :: tools/llvm-cov/ignore-filename-regex.test
LLVM :: tools/llvm-objdump/ELF/AMDGPU/source-lines.ll

These tests no longer fail.

The problem is that you're trying to do it all at once. There's no need to implement the full disassembly at once. You can do it in a series of patches, one after the other that build towards the primary goal. For example, there are many if (<X is some value>) type cases, which you could omit - just assume those are all false, in earlier versions, and add them in (with corresponding testing) in a later patch. Similarly, you could assume that all results of x & y return some specific value, e.g. 0, and just print that for now. Yes, that means you won't support everything from the point at which this patch lands, but it will make each individual patch easier to reason with. This fits much better with LLVM's preferred approach - please see https://llvm.org/docs/DeveloperPolicy.html#incremental-development.

Oh, you could also put the additional behaviour behind a switch in llvm-objdump. That'll mean you won't break all the existing tests too. Once you've finished the development, you could either remove the switch entirely or just change its default (which would allow users to disable the behaviour if they want the less verbose version of the output).

Thanks for the feedback! I'll keep these points in mind :)

Regarding this patch - I think that because it has been reviewed to quite some extent, it doesn't need to be broken into smaller patches now?

Maybe I missed something from some of the other reviewers, but I haven't seen much in the way of commentary on the core functionality of the new code, so I wouldn't say that it has been reivewed to quite some extent. I've personally only skirted around style aspects. I also don't see any evidence of any testing of the new code.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1486

Bits or bytes?

rochauha updated this revision to Diff 274016.Jun 29 2020, 2:11 AM

Added new test case.

rochauha marked 2 inline comments as done.Jun 29 2020, 5:52 AM
rochauha added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1486

Bits.

If there is a bit that is wrong in a particular chunk of bytes, we consider that the entire chunk of bytes is invalid. We then update the Size value.

Further, we say that the first Size bytes in a symbol are invalid. Error handling in llvm-objdump will print these bytes using .byte directive. And then we fall back to decoding the remaining bytes in the symbol as instructions.

rochauha marked 2 inline comments as done.Jun 29 2020, 7:01 AM
rochauha added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1486

We do this because most directives in the kernel descriptor affect single or a very few bits.

No idea about the functional logic of this code, but I do wonder whether you'd be better off dividing it into smaller patches for testability.

This patch is for entirely disassembling the kernel descriptor symbol. It happens to be so that many directives affect the kernel descriptor. I will also be adding a new test in this patch.

The problem is that you're trying to do it all at once. There's no need to implement the full disassembly at once. You can do it in a series of patches, one after the other that build towards the primary goal. For example, there are many if (<X is some value>) type cases, which you could omit - just assume those are all false, in earlier versions, and add them in (with corresponding testing) in a later patch. Similarly, you could assume that all results of x & y return some specific value, e.g. 0, and just print that for now. Yes, that means you won't support everything from the point at which this patch lands, but it will make each individual patch easier to reason with. This fits much better with LLVM's preferred approach - please see https://llvm.org/docs/DeveloperPolicy.html#incremental-development.

I'm curious if this is the intent of the document you linked, though? It says "The remaining inter-related work should be decomposed into unrelated sets of changes if possible.", but the disassembly of this directive is not decomposable into unrelated changes; if only part of the descriptor is disassembled then the whole .amdhsa_kernel block is invalid. I suppose in the interest of review one could break down the logical/atomic series of commits even further into inter-dependent patches that must be applied together? I think that causes confusion when doing git-bisect etc. so doesn't seem ideal. Or maybe the patches should be squashed back together after review but before they are committed?

I do think there needs to be more direct testing of the branches taken in the code. If decomposing the patch beyond the logical/atomic decomposition makes it easier to do that as an author and/or a reviewer then I am OK with it. In this case, though, a lot of the length in terms of SLOC is just repetition and a little redundancy, so breaking it up further would only hide that and make it harder to see where things are best removed or factored out.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1237

This doesn't seem right. This symbol is not accurate without careful attention from an assembly author, so it will surely be incorrect when used by a disassembler.

We can accurately compute a VGPR count from the GRANULATED_WORKITEM_VGPR_COUNT, we just calculate e.g. (GRANULATED_WORKITEM_VGPR_COUNT + 1) * granularity where the + 1 is needed to account for the minimum allocation (i.e. a granulated count encoded as "0" actually indicates a 1 granule allocation) and where the scaling by granularity is device-specific. This will calculate the greatest value possible for .amdhsa_next_free_vgpr which will produce the same granulated count, but equally valid you could choose to calculate the minimum, or any value in between. All we care about is producing disassembly which results in the same descriptor. (Caveat: I would prove this to yourself by referencing https://llvm.org/docs/AMDGPUUsage.html#amdgpu-amdhsa-compute-pgm-rsrc1-gfx6-gfx10-table as I may have the actual calculation wrong; we just want to calculate the inverse of what the assembler does)

Your point that we can't recover the exact value the original author intended to place in the assembly text is true, but that is OK as long as we can always give them a valid input to the assembler which gives them the same output.

1242

This can be a char literal, i.e. '\n', same elsewhere.

1255

For this and the above case we should have tests to prove this out. I.e. assemble sources to a binary, disassemble and reassemble it, and then compare the two binaries. Ideally we would do this for some edge cases around VGPR/SGPR allocation granularity.

There may need to be some fixup between disassembly and reassembly to account for the remaining non-reassembleable bits produced by llvm-objdump, but they should be pretty minor for a trivial kernel, and I would expect you could handle them with just sed which seems to be available to LIT tests.

1264

Just as with the VGPR count, we cannot use the symbol to define this, and we can compute a (non-unique) input which produces the same output.

1274

This shift isn't necessary, you just need to check for the presence of any set bits.

I also noticed when checking the types here that for some reason we declare the enum for these masks/shifts as int32_t, which makes one have to think about both integer promotion rules and then possibly which bitwise-operations are valid for signed integers. I think here the signed mask is promoted to unsigned, and you get what you want, but it may be good to go fix the definition of the masks separately.

1279

I think a short-lived well-defined preprocessor macro could make the patch shorter and easier to read. For example:

#define PRINT_TRIVIAL_FIELD(DIRECTIVE, MASK)  do { \
  KdStream << Indent << DIRECTIVE " " << ((FourByteBuffer & amdhsa:: ## MASK) >> amdhsa:: ## MASK ## _SHIFT) << '\n';
  } while (0);
PRINT_TRIVIAL_FIELD(".amdhsa_float_round_mode_32", COMPUTE_PGM_RSRC1_FLOAT_ROUND_MODE_32)
PRINT_TRIVIAL_FIELD(".amdhsa_float_round_mode_16_64", COMPUTE_PGM_RSRC1_FLOAT_ROUND_MODE_16_64)
// ...
#undef HANDLE_TRIVIAL_FIELD

I think in part this is because the definition of the masks themselves is in terms of preprocessor macros.

1300

Unnecessary shift

1311

Unnecessary shift, same for all other cases below when checking that the bits under a mask are 0.

1355

I don't know the general conventions here, but I don't think I have seen a comment for the end of a function elsewhere in LLVM. I do know that it is required for namespaces, so maybe it is permitted for long functions?

1397

All of these comments seem redundant to me, especially when the condition is simplified to just:

if (Buffer & MASK) return Fail;

At the very least, repeating the actual bit indices here when they are a part of the mask definition seems verbose.

1482

I don't understand the intent with carefully maintaining Size for the failure case. Aren't we certain by this point that this should be a kernel descriptor, and so the correct thing to do when we fail is to disassemble everything as .byte directives (i.e. set Size to the max value)? Why would we prefer to return a partial failure and have the disassembler start working on the remaining bytes as if they were instructions?

That would also shorten the patch and make it obvious that the Size tracking is correct.

1494

Rather than have these comments, which are still just filled with magic numbers, could we define these offsets more explicitly somewhere, as e.g. amdhsa::GROUP_SEGMENT_FIXED_SIZE_OFFSET? For example in llvm/include/llvm/Support/AMDHSAKernelDescriptor.h alongside the other definitions needed by the compiler? We could then also update the static_asserts there to use those definitions, so we aren't relying on inspection to know the same offsets are used everywhere. I.e. the following:

165 static_assert(
  1     sizeof(kernel_descriptor_t) == 64,
  2     "invalid size for kernel_descriptor_t");
  3 static_assert(
  4     offsetof(kernel_descriptor_t, group_segment_fixed_size) == 0,
  5     "invalid offset for group_segment_fixed_size");
  6 static_assert(
  7     offsetof(kernel_descriptor_t, private_segment_fixed_size) == 4,
  8     "invalid offset for private_segment_fixed_size");
  9 static_assert(
 10     offsetof(kernel_descriptor_t, reserved0) == 8,
 11     "invalid offset for reserved0");
...

Becomes:

165 static_assert(
  1     sizeof(kernel_descriptor_t) == 64,
  2     "invalid size for kernel_descriptor_t");
  3 static_assert(
  4     offsetof(kernel_descriptor_t, group_segment_fixed_size) == GROUP_SEGMENT_FIXED_SIZE_OFFSET,
  5     "invalid offset for group_segment_fixed_size");
  6 static_assert(
  7     offsetof(kernel_descriptor_t, private_segment_fixed_size) == PRIVATE_SEGMENT_FIXED_SIZE_OFFSET,
  8     "invalid offset for private_segment_fixed_size");
  9 static_assert(
 10     offsetof(kernel_descriptor_t, reserved0) == RESERVED0_OFFSET,
 11     "invalid offset for reserved0");
...
1640

The != 0 here is redundant.

1645

Could you move the call to .drop_back(3) out into the calling function, so it appears next to the check for .endswith(StringRef(".kd"))?

1675

I'm still not sure what we landed on for the semantics of SoftFail here?

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
83

Could you either rename this to satisfy the linter, or else explicitly suppress the lint with a comment like:

// NOLINTNEXTLINE(readability-identifier-naming)
llvm/test/tools/llvm-objdump/ELF/AMDGPU/code-object-v3.ll
48 ↗(On Diff #274016)

I think we need more tests to ensure:

  • The edge cases of the granularity calculation are correct
  • That our promise of round-trip is fulfilled for at least some representative cases
  • That some of the failure modes are handled how we expect

Updating the code bit by bit based on comments by @scott.linder.

rochauha updated this revision to Diff 275067.Jul 2 2020, 4:51 AM
rochauha marked 4 inline comments as done.
  • Used macro to shorten printing directives
  • Replaced "\n" with '\n'
  • Removed unnecessary bit shifts for bits that must be 0
  • Removed extra comments for bits that must be 0
rochauha added inline comments.Jul 2 2020, 4:53 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1355

I'm not sure. I added those comments because these functions were getting quite long.

1640

I know, but I thought that it is more readable this way.

rochauha marked an inline comment as done.Jul 2 2020, 4:56 AM
rochauha added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1675

It should be Success / Fail based on what the bytes are for code object v2. But there's nothing we are 'doing' at the moment for v2, I returned SoftFail.

rochauha marked an inline comment as done.Jul 2 2020, 5:02 AM
rochauha added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1255

Right now we can't really re-assemble in the lit-test. This needs to be tested 'informally' by:

  • Manually writing a small test case. Make a copy of it too.
  • Assembling it into the binary : Binary-1.
  • Disassembling it.
  • Replace the original kernel descriptor with the disassembled kernel descriptor in the copy.
  • Assemble the copy : Binary-2.
  • Compare Binary-1 and Binary-2.
rochauha updated this revision to Diff 275310.Jul 3 2020, 12:33 AM
rochauha marked 6 inline comments as done.
  • Fixed code-object-v3 lit test failure introduced in the previous change to this patch.
  • More changes as per inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1482

My initial take was to decode as byte directives to the point of failure indicated by Size. Then going back to the normal flow of disassembling as instructions.

But I get what you want to say in this comment.

Updated the code based on this comment.

1645

Done.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
83

Done.

rochauha updated this revision to Diff 275321.Jul 3 2020, 1:06 AM

Size = 64 regardless of success or failure.

rochauha updated this revision to Diff 275674.Jul 6 2020, 4:58 AM
  • Compute .amdhsa_next_free_vgpr based on inverse of what the assembler does to compute GRANULATED_WORKITEM_VGPR_COUNT.
  • Some changes to accomodate differences between GFX9 and GFX10
  • Updated test case for GFX10 as well
rochauha marked an inline comment as done.Jul 6 2020, 5:07 AM
rochauha added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1255

Went this route to check whether re-assembled binaries match or not. Turns out that both binaries match, in size (overall size as well as size of sections) and also in terms of all the disassembled content. But a diff object1 object2 says that binary files differ.

scott.linder added inline comments.Jul 6 2020, 1:06 PM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1255

I'm not sure I follow what you are describing; my thought was to start with just an asm source file containing only the kernel descriptor directive in the default section, and compare the output of the following (with, e.g. diff, as you mention):

  • Assemble it to an object file with llvm-mc
  • Assemble it to an object file with llvm-mc | disassemble the kernel descriptor symbol | trim any human-readable prologue | assemble it to an object file with llvm-mc

As a trivial example, diff doesn't find any difference for the following example:

$ printf '.amdhsa_kernel my_kernel\n.amdhsa_next_free_vgpr 0\n.amdhsa_next_free_sgpr 0\n.end_amdhsa_kernel' >a.s
$ release/bin/llvm-mc --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj a.s >a.o
$ diff a.o \
       <(release/bin/llvm-objdump --triple=amdgcn-amd-amdhsa --mcpu=gfx908 --disassemble-symbols=my_kernel.kd a.o \
                 | tail -n +8 \
                 | release/bin/llvm-mc --triple=amdgcn-amd-amdhsa -mcpu=gfx908 -filetype=obj)

I don't think you need to use FileCheck for these tests at all, you can just rely on ending the RUN pipeline with diff, which seems to be supported by lit. You can then just copy-paste the test and edit fields in the input to validate edge cases for things like the SGPR/VGPR allocation directives.

I think more comprehensive testing, including for other sections and executables/DSOs, would be good eventually but for now we should at least have some tests that explicitly confirm the KD disassembly round-trips.

1355

I would lean towards omitting these, especially with the functions becoming shorter. For example, decodeCOMPUTE_PGM_RSRC2() is now <50 lines long at the entire definition now fits on one screen for me. It seems like there are other examples of this in the codebase, though, so I'm OK with it for the longer functions.

1359

Can you expand this comment a little and move it to a Doxygen comment for the function?

1634

Need to handle the "default" case here:

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1510:1: warning: control may reach end of non-void function [-Wreturn-type]
1640

Fair enough, in a type-safe language it would be required anyway, so it seems reasonable.

1675

If SoftFail isn't applicable I don't think we should return it, even if it is just because we haven't implemented something yet. It existing doesn't mean it needs to be used, I think it has a very narrow definition that doesn't apply here.

Maybe just emit a diagnostic and return Fail so we get the "decode as .byte" behavior? What exactly happens now with the current patch as-is?

rochauha updated this revision to Diff 276059.Jul 7 2020, 7:28 AM
  • Handled default statement to silence the warning.
  • Expanded comments for decodeCOMPUTE_PGM_RSRC1 and decodeCOMPUTE_PGM_RSRC2.
  • Removed extra comment at the end of functions.
  • Changed SoftFail to Success for code object v2.
  • Replaced the old test case with a small assembly file.
rochauha updated this revision to Diff 276063.Jul 7 2020, 7:42 AM
rochauha marked 5 inline comments as done.
  • Return MCDisassembler::Fail for code object v2.
  • Add missing full stops in doxygen comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1355

Done.

1359

Done.

1634

Done.

1675

Done.

jhenderson added inline comments.Jul 8 2020, 1:19 AM
llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s
1 ↗(On Diff #276063)

No need for the <. llvm-mc is quite capable of taking inputs on the command-line as positional arguments.

2 ↗(On Diff #276063)

This line is too long. Please break it up into individual lines:
; RUN: llvm-objdump ... | \
; RUN: tail -n +8 | llvm-mc ...

5–8 ↗(On Diff #276063)

This test is also quite small. Does it actually cover every code path?

10 ↗(On Diff #276063)

Is this a FIXME/TODO? If so, please add "FIXME" or "TODO".

rochauha updated this revision to Diff 277360.Jul 13 2020, 2:55 AM

Changes as per review by @jhenderson.

rochauha marked 7 inline comments as done.Jul 13 2020, 3:03 AM
rochauha added inline comments.
llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s
2 ↗(On Diff #276063)

Done.

5–8 ↗(On Diff #276063)

These values must be always specified by the user. Values of some bytes are computed using the values passed here. Consequently there are some cases where we need to test for getting the exact same bytes in the re-assembled binary, even if the disassembled values slightly deviate from the original values.

Other bytes/bits hold the exact values specified using the assembler directives. Also they take default values if nothing is specified by the user. So I guess we don't need to test that for re-assembly.

10 ↗(On Diff #276063)

Done.

rochauha added inline comments.Jul 13 2020, 3:03 AM
llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s
1 ↗(On Diff #276063)

Done.

rochauha updated this revision to Diff 279490.Jul 21 2020, 5:15 AM
rochauha marked 3 inline comments as done.

Switched to disassembling as numerical value rather than .amdgcn.next_free_sgpr.

scott.linder requested changes to this revision.Jul 21 2020, 3:28 PM
scott.linder added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1253–1260

I think this can just be replaced with:

NextFreeVGPR = (GranulatedWorkitemVGPRCount + 1) * getVGPRAllocGranule(STI);

Or we could add another function called getNumVGPRs(const MCSubtargetInfo *STI, unsigned NumVGPRBlocks, Optional<bool> EnableWavefrontSize32) and put the definition directly next to getNumVGPRBlocks(const MCSubtargetInfo *STI, unsigned NumVGPRs, Optional<bool> EnableWavefrontSize32) so any future changes affecting one also affect the other. I would lean towards this, and documenting that they are the inverse of one another.

1288–1295

Same as above, I think a new getNumSGPRs to complement getNumSGPRBlocks would make this easier to read.

For the GFX10 case we could either leave the check here, or have the new function return Optional to indicate when there is an error.

1300

What is "GS" and why is this commented out?

1494

I would still like to see this done, the magic numbers here could lead to a few problems down the line, and presently they just make the code harder to read.

1582–1586

Can this just be return decodeKernelDescriptor(...);?

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s
6 ↗(On Diff #279490)

Can you implement more tests? I don't know if it is feasible to include them all in the same .s file, as you have to work around the output not being re-assembleable in general, but even just copy-pasting and editing is fine with me. Just having some more test cases to cover at least a reasonable sample of the different branches, failure modes, etc.

This revision now requires changes to proceed.Jul 21 2020, 3:28 PM
rochauha updated this revision to Diff 281180.Jul 28 2020, 3:58 AM
rochauha marked an inline comment as done.
  • Got rid of raw number and added an enum for offsets.
  • Added new tests.
  • Updated the nop-data.ll test so that it passes with this patch.
rochauha added inline comments.Jul 28 2020, 3:59 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1253–1260

I think this route is good for now :
NextFreeVGPR = (GranulatedWorkitemVGPRCount + 1) * getVGPRAllocGranule(STI);

Similarly for SGPRs.

I'd like to add the new functions via a separate patch. This patch is already quite big in terms of size.

1288–1295

As I mentioned in my other reply, this patch is already quite big. So it'd be better to have a separate patch for the new functions.

1300

This was was for printing Granulated wave front SGPR count.

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s
6 ↗(On Diff #279490)

I have added new tests in separate files.

scott.linder accepted this revision.Jul 30 2020, 9:40 AM

LGTM, thank you!

I'm probably not in a position to review the majority of this further. However, I do have big reservations about the testing - there are a high number of possible code paths, but I only see 5 tests, which clearly can't cover all these code paths.

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor-1.s
1 ↗(On Diff #281180)

It would be helpful to add a small comment at the start of each of these tests explaining what the are specifically testing.

I'm probably not in a position to review the majority of this further. However, I do have big reservations about the testing - there are a high number of possible code paths, but I only see 5 tests, which clearly can't cover all these code paths.

I think some of the difficulty in writing tests for the failure cases is not having a tool to produce them. I think one would have to generate the code object, and then hand-edit it in a hex-editor, copy it into the source tree (i.e. under Inputs/) maybe with some accompanying comment about the nature of the original source and the edits made?

It would be nice to at least cover some of the failure cases, even if they are more awkward to tests. There could also be some similar tests for e.g. GFX7,GFX10 and the edge cases around the {S,V}GPR calculation for each.

I'm probably not in a position to review the majority of this further. However, I do have big reservations about the testing - there are a high number of possible code paths, but I only see 5 tests, which clearly can't cover all these code paths.

I think some of the difficulty in writing tests for the failure cases is not having a tool to produce them. I think one would have to generate the code object, and then hand-edit it in a hex-editor, copy it into the source tree (i.e. under Inputs/) maybe with some accompanying comment about the nature of the original source and the edits made?

It would be nice to at least cover some of the failure cases, even if they are more awkward to tests. There could also be some similar tests for e.g. GFX7,GFX10 and the edge cases around the {S,V}GPR calculation for each.

It seems to me that the format of the data structure is well-understood (otherwise you wouldn't be able to write code to disassemble it). In similar situations, e.g. the DWARF .debug_line parsing, we didn't rely on the built-in .file/.loc directives to generate our line table, and instead wrote it out by hand using .byte/.half/.long/.quad. It's not the prettiest of things of course, but it's better than code paths that never get exercised in tests. You could also use yaml2obj to do a generate the section with a raw hex blob, and/or write an array of bytes in a gtest unit test. The latter situation might be particularly useful because it would allow you to use the same "base" array, and modify individual bytes in individual test cases to check the behaviour for each.

rochauha updated this revision to Diff 285815.Aug 14 2020, 11:04 PM
  • Updated tests and added a failure test.
  • Checking error when cursor holds Error::success() while handling failure cases.
  • Removed Size from subroutines' signatures as we aren't tracking the size.

Can I consider this patch to be at NFC stage?

rochauha edited the summary of this revision. (Show Details)Aug 17 2020, 10:50 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2020, 8:19 PM
This revision was automatically updated to reflect the committed changes.
jhenderson reopened this revision.Aug 19 2020, 12:34 AM

Hi @rochauha,

I'm not sure this should have been pushed - nobody has reviewed your latest update, and I've had concerns about the testing prior to that, as stated. There is also a "Needs Revision" marker still outstanding, and I usually take that to mean that this shouldn't be pushed until the relevant reviewer is satisfied, regardless of what others have said (note that Phabricator highlighted that this patch landded in a "Needs Review" state).

Please could you revert, as there are potential assertion failures in my reading of this code, at least in the event you hit malformed input, in addition to the other issues raised.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1230–1235

This is not how to handle malformed input. This will result in an assertion in debug builds, which is equivalent to a crash, without any useful context to draw on, because you've not reported the error. More below.

1418

Rather than all these checkError calls, I'd expect to see a check of the Cursor followed by a return of MCDisassembler::Fail to indicate there was a problem.

1552–1558

If this loop terminates due to C being invalid, you don't want to fall out the bottom and return Success, I think. You'd want to check C after loop termination and return Fail. Alternatively, if you return Fail from the decodeKernelDescriptorDirective you can do cantFail(C.takeError()); after the loop.

Ideally, we'd actually report the error from C in the event of failure, but currently there's no way of communicating that back up to the caller.

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s
2

Please add a comment to the top of this test explaining what this test is actually testing.

4

Rather than writing text to another input file at run time in this way, you can use the new split-file tool to have all the input inline below, and split it up using the tool into multiple files.

12

Please don't mix comment markers within the same file. You use '//' here, but ';' everywhere else.

Additionally, new LLVM binutils tests tend to use double comment markers to indicate true comments as opposed to RUN/CHECK lines (i.e. ';;' in this context).

35–36

Please remove the additional blank lines at the end of file.

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-sgpr.s
2

Please follow the comments from kd-failure.s in all the tests (where applicable) too.

rochauha updated this revision to Diff 290241.Sep 7 2020, 4:39 AM

Changes based on comments by @jhenderson

  • Used llvm::cantFail(C.takeError()) to handle error.
  • Removed blank lines at the end of test files.
  • Streamlined beginning of comments.
  • Used split-file tool to separate SGPR and VGPR test cases.
rochauha marked 2 inline comments as done.Sep 7 2020, 4:41 AM
rochauha added inline comments.
llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s
4

In case of kd-failure.s, concatenation is necessary because the disassembled output will be .bytes, and the symbol information is needed to be get the same binary again. I feel that "printf'ing" to a file and concatenating with disassembled text is a simpler compared to split file and concatenating.

However, I have used the split-file tool to separate the S/VGPR test cases.

rochauha marked an inline comment as done.Sep 7 2020, 4:48 AM
rochauha added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1552–1558

Thanks for the pointer! I think cantFail seems to be all that is needed, because the cursor was holding Error::success in some cases, which needs to be 'checked' before moving further.

Since the kernel descriptor is well defined, all cases where failure needs to be handled are handled using MCDisassembler::Fail.

@kzhuravl, do you have any additional comments to add at all, since you rejected the original patch?

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1404

I think for self-documentation purposes, it would be helpful to assert that Bytes.size() == 64 here. I see that it is verified in the calling function but a) that's not obvious when looking at this function in isolation, and b) in the future, we don't want other places calling this code without that check, so the assert provides a backstop of sorts.

1552–1558

Ah, I missed that the input can't be truncated, so C can't get into a failure state itself. Thanks! (See also my comment about an assert above).

llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-failure.s
3
llvm/test/tools/llvm-objdump/ELF/AMDGPU/kd-sgpr.s
23

Nit: missing full stop.

rochauha updated this revision to Diff 290443.Sep 8 2020, 2:55 AM
rochauha marked 3 inline comments as done.

Changes based on comments by @jhenderson.

jhenderson accepted this revision.Sep 8 2020, 4:45 AM

No more comments from me, but please give @kzhuravl a chance to respond before pushing this patch.

kzhuravl accepted this revision.Sep 8 2020, 8:44 AM

LGTM, thank you!

This revision is now accepted and ready to land.Sep 8 2020, 8:44 AM
rochauha added a comment.EditedSep 8 2020, 11:22 AM

3 of the test cases - kd-sgpr.s, kd-vgpr.s, kd-zeroed-gfx10.s fail with the PowerPC buildbot (http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/53608). From what I understand, these tests should only run if AMDGPU target is built. The lit.local.cfg file specifies that.

3 of the test cases - kd-sgpr.s, kd-vgpr.s, kd-zeroed-gfx10.s fail with the PowerPC buildbot (http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/53608). From what I understand, these tests should only run if AMDGPU target is built. The lit.local.cfg file specifies that.

The PowerPC build bot just means a build bot that is running on a PowerPC host. It actually targets most targets. See the following excerpt from the CMake.

-- Targeting AArch64
-- Targeting AMDGPU
-- Targeting ARM
-- Targeting AVR
-- Targeting BPF
-- Targeting Hexagon
-- Targeting Lanai
-- Targeting Mips
-- Targeting MSP430
-- Targeting NVPTX
-- Targeting PowerPC
-- Targeting RISCV
-- Targeting Sparc
-- Targeting SystemZ
-- Targeting WebAssembly
-- Targeting X86
-- Targeting XCore

I've run into this sort of problem before. The issue is almost certainly either a) incorrect assumption about host system endianness, meaning that you've incorrectly/inadvertently assumed the host is little endian, or b) assumed a 64-bit system somewhere. a) is almost certainly the issue here, based on both the test output and build bot name (ppc64*be*). I've skimmed the patch, but can't obviously see where the code is going wrong, but the test output for kd-zeroed-gfx10.s suggests it's around bytes 48-52. There may be other issues elsewhere though, since 0 renders the same regardless of size and endianness.

...

I've run into this sort of problem before. The issue is almost certainly either a) incorrect assumption about host system endianness, meaning that you've incorrectly/inadvertently assumed the host is little endian, or b) assumed a 64-bit system somewhere. a) is almost certainly the issue here, based on both the test output and build bot name (ppc64*be*). I've skimmed the patch, but can't obviously see where the code is going wrong, but the test output for kd-zeroed-gfx10.s suggests it's around bytes 48-52. There may be other issues elsewhere though, since 0 renders the same regardless of size and endianness.

Yes, considering the failures appear to be exactly the same on PowerPC Big Endian and SystemZ (which is also Big Endian), I would assume that there is a host endianness assumption here (I haven't looked at the code). Please pull this patch until a fix is ready. There are at least the 4 bots that are red because of this for quite a few consecutive builds. Pull the patch so we can get the bots back to green and we can work on a fix afterwards.

rochauha added inline comments.Oct 5 2020, 4:08 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1405

I think this is where little endian is being 'hardcoded'. However, since AMDGPU relocatable objects are meant to be little endian, I don't understand why they are big endian on pp64.

scott.linder added inline comments.Oct 5 2020, 12:04 PM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1405

I think this is actually a bug with the encoding code. Like you say, we should be host-endianness agnostic when encoding the kernel descriptor, but it seems we aren't.

I think I vaguely remember this coming up when we implemented it, but I don't remember why we didn't do this to start. I think it is just an oversight.

scott.linder added inline comments.Oct 5 2020, 3:55 PM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1405

I think https://reviews.llvm.org/D88858 should be the fix, need to confirm if big-endian testers will run it automatically.

scott.linder added inline comments.Oct 6 2020, 10:46 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1405

I tried to determine if the pre-checkin builders include any big-endian archs, but gave up and just committed it. I'll keep an eye on the builders and see if it needs to be revered. After that you can proceed with this patch again.

rochauha added inline comments.Oct 6 2020, 10:52 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1405

Thanks!

scott.linder added inline comments.Oct 6 2020, 1:42 PM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1405

An update, I missed one test in the initial commit, but I followed up in bf5c1d92d92ef8cee2adbfa17ecca20a8f65dc0e and now the big-endian testers seem to be happy. I think you can try reapplying this.

rochauha reopened this revision.Oct 6 2020, 8:44 PM

Previous reversions were due to test cases failing on big endian hosts.
This was due to multibyte values being laid out as-is from host memory.
Now https://reviews.llvm.org/D88858 addresses the above, and has landed.

This revision is now accepted and ready to land.Oct 6 2020, 8:44 PM
rochauha updated this revision to Diff 296590.Oct 6 2020, 9:08 PM

Re-applied patch.

scott.linder accepted this revision.Oct 7 2020, 8:08 AM
This revision was landed with ongoing or failed builds.Oct 7 2020, 8:11 AM
This revision was automatically updated to reflect the committed changes.