smithp35 wrote: Thanks for expanding the commit message. I really don't know where to start with this, apologies if this doesn't read coherently. It may be that we need to take a step back and perhaps start a RFC on discourse to get some more visibility.
Firstly, there's a huge amount of code here and not a lot of accessible design/specification. Without some consensus that this is the right design approach [1] I'm reluctant to spend a lot of time on the code. I'm also reluctant to give out suggestions that would involve a lot of work on your part if there's blocking objections to the design. That is wasting your time too. While I understand that linker's have never been 100% adherent to a vanilla ELF specification of sections, symbols and relocations, there's still quite a bit about this proposal that makes me nervous about this in its current form. To keep it as short as possible: * Linkers rely on the processor specific ELF supplement that defines processor specific ELF extensions. There should be a document defining all the data structures that are encoded in the ELF file, for example https://github.com/riscv-non-isa/riscv-elf-psabi-doc for RISC-V and https://github.com/ARM-software/abi-aa/tree/main/aaelf64 for AArch64. With just the source code commit message and PR description we have no way of knowing whether what is in here is implemented correctly. * How stable is this ABI? Is this the format that AMD is choosing for all of its kernels going forward, or will it change between different models? I don't think we should always rule out changes because they are complex, but if code is likely to change or grow a lot then it likely belongs outside upstream lld. * Skimming the code there looks to be references to ordering constraints between sections, and patching of sections. Could these be made explicit in a design document? I'm wondering if there is an alternative encoding in the smart format, dumb linker design ethos of ELF that lets us make more use of sections, symbols and relocations to do the patching. I think there is some scope to split the patch up. It would require some `--print-amd-*` options to serialise enough the output to test the linker, but with that I think you could do something like: * Test the reading and decoding of the custom AMDGPU sections. * Test the validation code prior to any graph construction. * Test the implementation of the generic graph algorithms [3], perhaps even using GoogleTest. [1] For example; a separate tool that collates the information, a LTO like interface logically equivalent to a plugin, perhaps an alternative encoding of the information in the ELF file. There could also be a case for keeping this downstream, at least until there's a second GPU like device needing similar support. [2] The only public reference to smart format, dumb linker I can find is a presentation I gave myself. [3] Clang has a callgraph ELF section extension https://llvm.org/docs/CallGraphSection.html perhaps some of the kernel callgraph could be encoding using this extension, and the code to construct a callgraph made generic. That would permit general analysis like checking maximum stack usage of a chain of functions. https://github.com/llvm/llvm-project/pull/206787 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
