- Add target id support (https://clang.llvm.org/docs/ClangOffloadBundler.html#target-id)
- Add code object v4 support (https://llvm.org/docs/AMDGPUUsage.html#elf-code-object)
- Add kernarg_size to kernel descriptor
- Change trap handler ABI to no longer move queue pointer into s[0:1]
- Cleanup ELF definitions
- Add V2, V3, V4 suffixes to make a clear distinction for code object version
- Consolidate note names
Details
Diff Detail
Event Timeline
llvm/lib/Target/AMDGPU/AMDGPU.td | ||
---|---|---|
873 ↗ | (On Diff #319953) | This looks like a separate change |
llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp | ||
---|---|---|
129 | Why check getTargetStreamer()? Why would htis ever be null? The uses below don't check it | |
207–208 | Probably should use proper context errors here | |
627 | Should use a temp var for getTargetStreaemr()->getTargetId() instead of repeating it so many times | |
llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
5121–5124 | Weird naming scheme. lowerTrapHSAQueuePtr? | |
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp | ||
381–390 | StringSwitch? Also check the prefix and drop gfx? |
I've only looked at the llvm-readobj stuff: there are a very large number of changes to that tool in this change, but no direct testing (i.e. tests under llvm\test\llvm-readobj) that has been changed. I'm guessing it's not all covered by existing direct testing of the tool?
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
5011 | Whilst I follow what's going on here after reading more carefully, the single byte array is confusing to me. Is there a particular reason for doing it this way, rather than just omitting it and using Desc.data() + sizeof(IsaVersion)? The latter seems more obvious to me. | |
5052 | i -> I. Also LLVM style is to precalculate the end condition where possible. See inline edit. | |
5055 | Could you just return MetadataString directly? Similar comment in other cases. | |
6115 | It seems to me like there's potential for other versions either now or in the future that don't support the V3 flags? Is there a risk this default case will be unintentionally hit in those cases? |
It is not covered by the direct testing of the tool (no tests in llvm\test\llvm-readobj), but has a very good coverage in llvm/test/CodeGen/AMDGPU.
llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp | ||
---|---|---|
129 | Check is here because the following test is passing "-emit-codegen-only" option to cc1, which executes EmitCodeGenOnlyAction, which does not have MC including target streamer. Clang :: Misc/backend-resource-limit-diagnostics.cl The uses below never check it because backend-resource-limit-diagnostics.cl does not specify the os in the triple. Do you have better suggestions on how to work around it? | |
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp | ||
381–390 | Do you suggest converting the whole if-else block into StringSwitch? If yes, how would errors be handled? In addition there is an else statement saying we do not support the processor in v2: ... } else if (Processor == "gfx906") { if (isXnackOnOrAny()) Processor = "gfx907"; } else { report_fatal_error( "AMD GPU code object V2 does not support processor " + Processor); } | |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
5011 | I guess this was done to convey more readability. But since it failed, I have changed to the way you suggested. | |
6115 | I think being more explicit is good. Thanks! |
lld/ELF/Arch/AMDGPU.cpp | ||
---|---|---|
17 | Consider posting lld/ELF changes separately. It is loosely related to the main change. | |
47 | Delete this helper - it is only used once. | |
55 | Delete | |
59 | Delete | |
63 | Delete | |
101 | delete braces around simple statements | |
106 | delete parens | |
115 | delete braces | |
132 | If the value is possible (malformed input), use error instead of unreachable don't capitalize messages |
llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp | ||
---|---|---|
129 | I don't understand why the asm printer would ever execute without the streamer. I don't see why the triple would matter, or why it matters for this specific test. What happens if you add a triple to that test? |
llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp | ||
---|---|---|
129 |
-emit-codegen-only option results in no streamer being created
If amdhsa or amdpal is not specified, this function is a NOP. If I add amdhsa or amdpal to triple in the backend-resource-limit-diagnostics.cl test, then it segfaults (without D95638) I have modified the test to not specify -emit-codegen-only option, which allows us to remove the check here. |
Usually we have testing for llvm-readobj behaviour in test/tools/llvm-readobj, so that it can be kept independent of other testing, and so that changes in other areas don't impact test coverage for llvm-readobj. It also makes it easier to keep the tests focused on individual features. For example, what testing is there for EM_AMDGPU objects with an unrecognised ABI Version, or other aspects of that switch?
llvm/test/tools/llvm-readobj/ELF/note-amd.s | ||
---|---|---|
27 | What are you trying to achieve with these checks? That there is explicit whitespace on this line and nothing else? That seems less than ideal to me. Why would you want that? | |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
6115 | You need to print an empty Flags field here, for consistency with other output. |
The test for unrecognized ABI version is missing, thanks for catching this, it will be included in the newer patchset I am currently working on.
We do have numerous tests that rely on llvm-readobj (and particularly that switch) to test e_flags in codegen, yaml, asm, and lld. Couple of examples:
llvm/test/CodeGen/AMDGPU/tid-one-func-xnack-any.ll llvm/test/CodeGen/AMDGPU/tid-mul-func-xnack-any-on-1.ll llvm/test/CodeGen/AMDGPU/tid-mul-func-xnack-any-off-2.ll llvm/test/CodeGen/AMDGPU/elf-header-flags-xnack.ll llvm/test/CodeGen/AMDGPU/tid-mul-func-xnack-all-off.ll llvm/test/CodeGen/AMDGPU/tid-mul-func-xnack-all-on.ll llvm/test/CodeGen/AMDGPU/tid-mul-func-xnack-all-any.ll llvm/test/CodeGen/AMDGPU/tid-mul-func-xnack-any-on-2.ll llvm/test/CodeGen/AMDGPU/tid-one-func-xnack-off.ll llvm/test/CodeGen/AMDGPU/elf-header-flags-sramecc.ll llvm/test/CodeGen/AMDGPU/tid-one-func-xnack-on.ll llvm/test/CodeGen/AMDGPU/tid-mul-func-xnack-any-off-1.ll llvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml llvm/test/Object/AMDGPU/elf-header-flags-sramecc.yaml llvm/test/Object/AMDGPU/elf-header-flags-xnack.yaml
LLD test also uses llvm-readobj: https://reviews.llvm.org/D95811
Similar situation with notes.
Is the suggestion to add similar tests [maybe selective?] tests under test/tools/llvm-readobj?
llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp | ||
---|---|---|
129 | Added back null check as discussed offline. I will post a separate review to fix other places. | |
llvm/test/tools/llvm-readobj/ELF/note-amd.s | ||
27 | This check is here because we started processing *NT_AMD_PAL_METADATA* in *getAMDNote*, and if note's desc is empty (which is the case here), we are going to output an empty string (there is whitespace before returned empty string so we cannot use EMPTY). Similar checks were put in https://reviews.llvm.org/D96010 . Also see lines 11 and 14 above. Do you have suggestions on how to improve this? | |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
6115 | Thanks, will be in newer diff. |
llvm/test/CodeGen/AMDGPU/elf-header-flags-sramecc.ll | ||
---|---|---|
6 | Can you add gfx90a and gfx90c? |
llvm/test/CodeGen/AMDGPU/elf-header-flags-sramecc.ll | ||
---|---|---|
6 | Actually only gfx90a. |
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
---|---|---|
4516 | Why &&? I also have no idea what this type is, so I think the auto hurts | |
4517 | I'm guessing this is an Optional, in which case *HasAbiVer | |
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp | ||
4822 | The subtarget still has the register even if xnack isn't enabled |
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp | ||
---|---|---|
4822 | supported != enabled. isXnackSupported returns true for every ASIC that supports xnack, false otherwise. To check whether xnack is enabled or not, one can check the return value of getXnackSetting and see if it is "On": https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h#L108 |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
5052 | @kzhuravl - this is a good example of why I want llvm-readobj testing for llvm-readobj code. There's a bug in this code (which I unfortunately introduced with my previous suggestion) which should have been easily picked up if it was properly tested. It should be ++I not ++E. Please fix ASAP, as it is being flagged up by our downstream static analyzer. |
Apologies for not coming back to review this before. I've taken a look through the llvm-readobj changes, and highlighted a number of code paths which I don't think have been tested at all (based on the fact that strings that should be printed don't appear in any of the test changes with this patch). Please create a new patch to address these and my previous comment. If necessary, use yaml2obj to generate note sections with the appropriate metadata.
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
4974–4976 | This code appears to be untested. | |
4980–4981 | This code is untested. | |
4992–4993 | This code appears to be untested. | |
4997–5001 | This code is untested. | |
5012–5013 | This code appears to be untested. | |
5015–5018 | This code appears to be untested. | |
5021–5027 | This code is untested. |
Hi, sorry, I was out of office, and I just returned.
I did ping few weeks overall, submitted 1 week after this patch was accepted (thinking there may be additional feedback), and did not hear back from you.
I will address your comments and post a follow up patch today or tomorrow.
No problem, and sorry for abandoning the review originally - things got a bit hectic my end and I couldn't keep track of all the reviews.
Consider posting lld/ELF changes separately. It is loosely related to the main change.