[libc-commits] [PATCH] D139839: [libc] Add a loader utility for AMDHSA architectures for testing

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

I have not gone through every line of code and I am likely not the right person to review the GPU side of things. Few high level comments about organization:

1. I think what you are proposing here is the ability to add what we call as "integration tests" in the libc today. See https://github.com/llvm/llvm-project/tree/main/libc/test/integration. The idea behind an integration test is that the test executable contains only pieces from the libc. So, even the startup object like `crt1.o` come from the libc. Items like the thread libraries, whose implementation is tightly coupled to implementation of the startup system, are tested using integration tests. Things which can be tested without pulling the startup code are still tested using unit tests. If I understand your patch, you are proposing to use special startup code from the libc to run even the unit tests also. Which is probably implying that you want to run the unit tests also as integration tests on the GPU.
2. The Linux startup code currently is in the directory named `loader`: https://github.com/llvm/llvm-project/tree/main/libc/loader. I agree that the name "loader" is not appropriate. We have plans to change it to "startup". What you are calling as `amdhsa_crt` should live along side the linux startup code and follow the conventions followed by the linux startup code.

As I see it, the way forward for this patch is this:

1. I will rename the current `loader` directory to `startup` and you can move the GPU startup implementation there. As it stands in this patch, it is a trivial implementation so you can land it separately after I do the renaming.
2. You can land the `DeviceLoader` implementation as a next step - GPU/AMD experts should review that. Keeping it under `libc/utils` sounds good but may be it should live in a nested directory `libc/utils/gpu`.
3. As far as running the unit tests on the device, I think what we really want is the ability to be able to run the unit tests as integration tests. We might not be able to run all of the unit tests in that fashion - for example, the math unit tests use MPFR which I would think cannot be made to run on the GPU. So, we can do two different things here: i) Add some integration tests which can be run on the GPU as well as a Linux host. ii) Develop the ability to run any unit test as an integration test. We really want to be explicit in listing targets in the libc project so one approach could be to list integration test targets which point to the unit test sources. And, this can be done for a Linux host and the GPU.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139839/new/

https://reviews.llvm.org/D139839