LGTM, and can you please open a follow-up review once this is committed to hide all of the Object::get<Target><Stuff> methods?

I don't know that my opinion holds much weight, but I agree that reducing the number of nearly-equivalent-but-subtly-different intrinsics/pseudo-instructions for debug-information would make it easier for a new dev to understand their semantics, and make it much easier to extend. I would argue that using different representations based on complexity doesn't make the result any more comprehensible, it just adds more things to comprehend.

Looking good to me! Some small nits, and it would be good to hear back on what the general consensus on adding this to Object is.

In D84519#2200683, @jdoerfert wrote:

This seems to be related to the push of the cpu-id (aka. target-id) into other places as well: e.g., D84822, D80750, probably more.
I mentioned in those reviews already, this doesn't strike me as an "AMDGPU" feature at all.
We should start the discussions around missing functionality instead of adding AMDGPU workarounds in all these places...
One example is an IR module level target cpu and/or target feature, similar to target triple.

---

Also, the commit doesn't have a message that explains what is going on.

LGTM, thank you!

In D84519#2195705, @rochauha wrote:

In D84519#2176909, @scott.linder wrote:

In D84519#2174866, @MaskRay wrote:

I am a bit worrisome that this patch is adding more target specific stuff to llvm-objdump.

Agreed, I have looked at making the majority of llvm-objdump (and essentially everything under tools/) into a library at one point, but the biggest initial hurdle is the CommandLine parsing library. I think ideally all of the target-relevant parts would be in target-overridden implementations in some library, all of the llvm-objdump behavior parts (like the core symbol-to-symbol loop) would be in an llvm-objdump library which depends on the libraries with target-specific behaviors, and the tools/ directory would contain ~10 line main functions.

Can we step back and think whether precise CPU selection is really necessary? For example, PowerPC has the needs for different ISA levels but it simply uses -mcpu=future.

I am a bit lost on what you mean; is there a significant cost to providing precise CPU selection? It doesn't make sense to require the user to e.g. run the dumper to inspect the binary in order to come up with the command-line to run the dumper in order to inspect the binary.

If different AMDGPU CPUs are incompatible, should such detection be moved to somewhere in lib/Target/AMDGPU/?

Yes, I think it should be done in a generic way. All other targets which don't need/want it just getting the default (existing) behavior.

I'd like to point out that this patch correctly detects the CPU string by looking at the binary. The CPU string needs to be determined beforehand and passed on to the SubtargetInfo constructor.

To move the CPU string detection to lib/Target/AMDGPU, the target would also need to look at the binary. I think, one would have to pass ObjectFile * to the MCSubtargetInfo constructor. This allows targets to do the needful in their respective constructors. I'd like to know thoughts/opinions on this.

The other option is to treat any frame that can possibly have an offset that won't fit in the immediate field as needing an SP

I don't know if this has anything to do with David's issue, but I think separately this makes sense. I'm just confused what the semantic difference between the two "do we need an SP" predicates is?

In D80713#2186791, @jhenderson wrote:

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.

Sorry for the insane delay, this makes perfect sense to me.

LGTM, thank you!

In D84519#2177164, @MaskRay wrote:

In D84519#2176909, @scott.linder wrote:

In D84519#2174866, @MaskRay wrote:

Can we step back and think whether precise CPU selection is really necessary? For example, PowerPC has the needs for different ISA levels but it simply uses -mcpu=future.

I am a bit lost on what you mean; is there a significant cost to providing precise CPU selection? It doesn't make sense to require the user to e.g. run the dumper to inspect the binary in order to come up with the command-line to run the dumper in order to inspect the binary.

-mcpu=pwr10 decodes more instructions than -mcpu=pwr9.
-mcpu=pwr9 decodes more instructions than -mcpu=pwr8.

I think GNU objdump just decodes all instructions it recognizes, not expecting a -mcpu option. It behaves as if the default recognized ISA level includes everything.

In D84519#2174866, @MaskRay wrote:

I am a bit worrisome that this patch is adding more target specific stuff to llvm-objdump.

In D84194#2173959, @rochauha wrote:

In D84194#2170882, @scott.linder wrote:

I discussed with Tony today, and I was thinking about this the wrong way.

SPI does not require the granule count to be even, it just rounds up the granule count before actually performing the allocation. This means, from the compiler's perspective, when it is calculating things like the AMDGPU::IsaInfo::getMaxNumSGPRs it must consider the "allocation" granule size (IsaInfo::getSGPRAllocGranule). Conversely, from the assembler/diassembler perspective, it must consider the "encoding" granule size (IsaInfo::getSGPREncodingGranule). It is perfectly OK to have a GFX9 code object with a granulated SGPR count of 1, and we should allow emitting that in the assembler so that the disassembler can accurately reproduce those code objects.

I don't think there is any fix needed here, we already separate these two concepts and correctly apply them elsewhere. I think I just led you astray in the disassembly patch; you should only be using the encoding granule size, and shouldn't need any special handling for e.g. GFX9 to handle the fact that the allocation and encoding granule sizes are not equal.

Correct me if I'm wrong. So we must not take inverse of the mentioned GFX9 calculation (the one where we divide by 16 before roundup) as it is for allocation granule size? And hence the disassembly computation will be same for GFX6-8 and GFX9 (because the encoding granule size is the same)?

I discussed with Tony today, and I was thinking about this the wrong way.

In D84391#2170741, @scott.linder wrote:

Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were flatScratchIsHwreg() and it implied flatScratchIsPointer() this becomes:

if (ST.flatScratchIsHwreg()) {
  // pointer add then S_SETREG
} else if (ST.flatScratchIsPointer()) (
  // pointer add
} else {
  // size and offset dance
}
llvm_unreachable();

I agree that the existing assert here doesn't really mean anything, not sure why I wrote it.

Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were flatScratchIsHwreg() and it implied flatScratchIsPointer() this becomes:

In D80249#2158886, @nhaehnle wrote:

In D80249#2156889, @arsenm wrote:

In D80249#2156869, @nhaehnle wrote:

Do you really need to change the signature of MachineFunctionInfo::create? That seems orthogonal to the problem you're trying to solve, or perhaps a redundant solution.

The problem is: due to lazy construction and being able to refer to the MachineFunction, the state of the MachineFunctionInfo can have surprising accidental differences depending on when exactly it was created. There are two possible solutions:

1. Prevent the MachineFunctionInfo from accessing the MachineFunction.
2. Always create the MachineFunctionInfo in the same place.

This change does both, but either solution should be sufficient, and I personally find #1 counter-intuitive: it seems to me that the additional target-specific info about an object should be allowed to access that object.

You can't really do anything with the empty MachineFunction on construction. At the construction point, there isn't any information to access and it would always be a mistake to inspect it or make any construction decisions based on it (it would also be bad for the constructor to modify the MachineFunction).

MFI is mutable, and so referring back to the MachineFunction as part of the mutation is potentially something reasonable to do. So the point isn't to use data from the MachineFunction during construction of the MFI, but rather to allow the "child" object to have a backlink to its "parent".

In D84194#2164406, @rochauha wrote:

In D84194#2164227, @arsenm wrote:

Needs test

I think these changes are tested using the test in https://reviews.llvm.org/D80713.
In fact the issue was found when testing round tripping for the above patch. I guess this can only be verified by a round trip test when we assemble->disassemble->re-assemble? Such a test is already present in the patch for D80713.

I am not sure how else can we look at the value of GRANULATED_WAVEFRONT_SGPR_COUNT in a test case.

In D80249#2156869, @nhaehnle wrote:

Do you really need to change the signature of MachineFunctionInfo::create? That seems orthogonal to the problem you're trying to solve, or perhaps a redundant solution.

The problem is: due to lazy construction and being able to refer to the MachineFunction, the state of the MachineFunctionInfo can have surprising accidental differences depending on when exactly it was created. There are two possible solutions:

1. Prevent the MachineFunctionInfo from accessing the MachineFunction.
2. Always create the MachineFunctionInfo in the same place.

This change does both, but either solution should be sufficient, and I personally find #1 counter-intuitive: it seems to me that the additional target-specific info about an object should be allowed to access that object.

In an ideal world, we'd merge all the binary tools (GNU and LLVM) into a single tool, or redistribute functionality somehow, so that we don't have duplicate functionality like we already do. This takes us further away from that ideal.

Sorry for the delay in reviewing, I've tried to wrap my head around this and I'm still lost.

In D80713#2110898, @jhenderson wrote:

In D80713#2108650, @rochauha wrote:

In D80713#2106033, @jhenderson wrote:

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.

Simplify test

LGTM, sorry for the delay in reviewing. My feeling is that we should just keep the simple rules you have outlined in the docs and require the callback implementation ensure things are not decoded twice by buffering their output (if needed) and setting Size correctly.

I don't know why I didn't add any tests of getDesc() (or anything else) originally, maybe because there is no existing testing and it requires both a unit test and a binary file input? I think at this point we shouldn't block adding an obvious 1-line function on adding tests that should already exist. So, LGTM with Matt's request addressed, and we can add testing in a general way at some point.

LGTM, thanks!

I'm not comfortable accepting this as I'm not familiar with the code, but it LGTM. The pattern of constructing the MachineFunctionInfo lazily on the first call to getInfo seems very non-obvious; it isn't implied by the name and it isn't described in the doc comment. As you note it is also particularly dangerous if it depends on values which are mutable over the life of the MachineFunction.

LGTM, thanks!

LGTM, thank you for bearing with us taking a while to review. I left a small request but feel free to commit without updating the review.

In D79671#2041363, @tpr wrote:

With the nil/empty change, there is a change in behavior in that doing a map lookup "map[key]" with a key not previously in the map creates a new map entry that is empty rather than nil. That is more correct, but I have just discovered some code in LLPC relying on the old behavior. I can fix the LLPC code, but is the change in behavior going to cause any HSA problems?

I posted a piglit test to try to exercise this https://gitlab.freedesktop.org/mesa/piglit/-/merge_requests/291

I'd be interested in the answer concerning why we need to avoid git rev-parse HEAD; it seems like the cleanest solution is to just always check if git rev-parse HEAD changes to determine whether to regenerate the header.

Correct test file names. I missed this when renaming the CFA operation itself
to match the updated proposal.

Use llvm-readelf for more compact tests

• Use -COUNT-N form
• Use '#' comment character
• Use llvm-readelf for more compact tests
• Refactor lit config test

Refer to subreg index directly

This will require some changes to the CFI, although previously this case was broken anyway so it does not regress the unwind information. I think we will just need to define the CFA relative to the base pointer (if it is actually set up) or the stack pointer (if realignment is needed, but the base pointer is not).

Re-add a test that got lost in a rebase somewhere

Add tests for parse errors, fix formatting

LGTM, although CHECKs in the tests to confirm that e.g. the first involves a scavenged register and s_add, and the second involves the stack pointer and s_mov, and that this happens at the expected offset might make it easier to read and more likely to catch something that breaks this in the future. Right now it isn't clear why there are two nearly identical tests.

In D79671#2032032, @tpr wrote:

In D79671#2030708, @scott.linder wrote:

Dinesh is working on merging metadata in ROCm CompilerSupport using MsgPackDocument, and it fell out that merging YAML metadata was also handled transparently. What is the advantage to doing this via a function pointer passed to readFromBlob? It doesn't seem to save any copies as you still need to be able to compare the nodes; is it just a matter of readability? Would it be reasonable to also add a callback for fromYAML?

I too tried doing it by reading the second blob into a MsgPackDocument and merging it into the first one from there. But I think that does involve more copies/allocation/etc because a map in the second document would actually get created as a std::map before having its elements merged into the corresponding map in the first document.

I guess the same thing as I have done could be done for the YAML reading code.

Rebase over https://reviews.llvm.org/D79878

Rebase

Rebase

Rebase, generalize the previously hard-coded list of caller-saved registers
with help from @cdevadas, and emit CFI for the new case where we spill the FP
to VMEM.

Rebase

Rebase and update summary

Rebase and move tests to .debug_info as some new operations are not legal in
.debug_frame.

Rebase

Address feedback

Dinesh is working on merging metadata in ROCm CompilerSupport using MsgPackDocument, and it fell out that merging YAML metadata was also handled transparently. What is the advantage to doing this via a function pointer passed to readFromBlob? It doesn't seem to save any copies as you still need to be able to compare the nodes; is it just a matter of readability? Would it be reasonable to also add a callback for fromYAML?

In D78778#2029334, @RamNalamothu wrote:

@scott.linder is this not yet ready for review, as you have not added any reviewers?

Thank you for the explanation; I'm also still lost as to why .git/logs/HEAD is referenced at all then. I would propose removing find_first_existing_vc_file entirely, as it seems to be completely redundant if there is another mechanism for ensuring the VCS header is up-to-date without triggering gratuitous rebuilds, but I will defer to any of the other reviewers on the blame for the scripts.

Can you provide more context in the diff? Looking at the current master locally it seems like this patch changes the behavior of the llvm_vcsrevision_h target when logs/HEAD isn't found, such that it only depends on the generation script. I.e. if you configure CMake without the HEAD file present, and then you do something which changes the working directory and creates HEAD the target will not be out of date and won't be rebuilt. Before this patch it would have noticed the change to HEAD and forced regenerating the version header.