[PATCH] D95638: AMDGPU: Add target id and code object v4 support

Wed Feb 17 18:10:20 PST 2021

kzhuravl marked 2 inline comments as done.
kzhuravl added a comment.

In D95638#2564830 <https://reviews.llvm.org/D95638#2564830>, @jhenderson wrote:

> In D95638#2531247 <https://reviews.llvm.org/D95638#2531247>, @kzhuravl wrote:
>
>> In D95638#2530115 <https://reviews.llvm.org/D95638#2530115>, @jhenderson wrote:
>>
>>> 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?
>>
>> 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.
>
> 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?

The test for unrecognized ABI version is missing, thanks for catching this, I will add it once I upload a newer patch.

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:

  tid-one-func-xnack-any.ll
  tid-mul-func-xnack-any-on-1.ll
  tid-mul-func-xnack-any-off-2.ll
  elf-header-flags-xnack.ll
  tid-mul-func-xnack-all-off.ll
  tid-mul-func-xnack-all-on.ll
  tid-mul-func-xnack-all-any.ll
  tid-mul-func-xnack-any-on-2.ll
  tid-one-func-xnack-off.ll
  elf-header-flags-sramecc.ll
  tid-one-func-xnack-on.ll
  tid-mul-func-xnack-any-off-1.ll
  elf-header-flags-mach.ll
  elf-header-flags-mach.yaml
  elf-header-flags-sramecc.yaml
  elf-header-flags-xnack.yaml

Similar situation with notes.

Is the suggestion to add similar tests [maybe selective?] tests under test/tools/llvm-readobj?

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:129
+  // emitFunctionBodyStart?
+  if (getTargetStreamer() && !getTargetStreamer()->getTargetID())
+    initializeTargetID(M);
----------------
kzhuravl wrote:
> arsenm wrote:
> > kzhuravl wrote:
> > > arsenm wrote:
> > > > Why check getTargetStreamer()? Why would htis ever be null? The uses below don't check it
> > > 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?
> > 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?
> > I don't understand why the asm printer would ever execute without the streamer.
> 
> -emit-codegen-only option results in no streamer being created
> 
> > 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?
> 
> 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.
> 
> https://reviews.llvm.org/D96728
Added back null check as discussed offline. I will post a separate review to fix other places.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-amd.s:29
+// GNU-NEXT:     AMD PAL Metadata:
+// GNU-NEXT: {{^        $}}
 // GNU-EMPTY:
----------------
jhenderson wrote:
> 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?
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?

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6209
+      default:
+        break;
+      case 0:
----------------
jhenderson wrote:
> You need to print an empty Flags field here, for consistency with other output.
Thanks, will be in newer diff.

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

https://reviews.llvm.org/D95638