This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Add target id and code object v4 support
ClosedPublic

Authored by kzhuravl on Jan 28 2021, 1:52 PM.

Details

Summary

Diff Detail

Event Timeline

kzhuravl created this revision.Jan 28 2021, 1:52 PM
kzhuravl requested review of this revision.Jan 28 2021, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 1:52 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Jan 28 2021, 1:58 PM
llvm/lib/Target/AMDGPU/AMDGPU.td
873 ↗(On Diff #319953)

This looks like a separate change

arsenm added inline comments.Jan 28 2021, 1:58 PM
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

652

Should use a temp var for getTargetStreaemr()->getTargetId() instead of repeating it so many times

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
5186–5189

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
5044

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.

5085

i -> I. Also LLVM style is to precalculate the end condition where possible. See inline edit.

5088

Could you just return MetadataString directly? Similar comment in other cases.

6193

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?

kzhuravl marked 10 inline comments as done.Jan 29 2021, 1:14 PM

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.

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
5044

I guess this was done to convey more readability. But since it failed, I have changed to the way you suggested.

6193

I think being more explicit is good. Thanks!

kzhuravl updated this revision to Diff 320200.Jan 29 2021, 1:16 PM
kzhuravl marked 4 inline comments as done.

Address review feedback.

MaskRay added inline comments.Jan 29 2021, 6:23 PM
lld/ELF/Arch/AMDGPU.cpp
17 ↗(On Diff #320200)

Consider posting lld/ELF changes separately. It is loosely related to the main change.

47 ↗(On Diff #320200)

Delete this helper - it is only used once.

55 ↗(On Diff #320200)

Delete

59 ↗(On Diff #320200)

Delete

63 ↗(On Diff #320200)

Delete

101 ↗(On Diff #320200)

delete braces around simple statements

106 ↗(On Diff #320200)

delete parens

115 ↗(On Diff #320200)

delete braces

132 ↗(On Diff #320200)

If the value is possible (malformed input), use error instead of unreachable

don't capitalize messages

kzhuravl updated this revision to Diff 320558.Feb 1 2021, 12:23 PM
kzhuravl marked 9 inline comments as done.

Address review feedback.

lld/ELF/Arch/AMDGPU.cpp
17 ↗(On Diff #320200)
tra added a subscriber: tra.Feb 1 2021, 2:07 PM
arsenm added inline comments.Feb 12 2021, 8:10 AM
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?

kzhuravl added inline comments.Feb 15 2021, 12:26 PM
llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
129

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

kzhuravl updated this revision to Diff 323817.Feb 15 2021, 12:27 PM

Rebase and remove streamer check in AMDGPUAsmPrinter.cpp

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?

llvm/test/tools/llvm-readobj/ELF/note-amd.s
29

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
6193

You need to print an empty Flags field here, for consistency with other output.

kzhuravl marked 2 inline comments as done.EditedFeb 17 2021, 6:10 PM

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, 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
29

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
6193

Thanks, will be in newer diff.

kzhuravl marked an inline comment as done.Feb 18 2021, 12:30 PM

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?

Updated llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-hea ders.test.

kzhuravl updated this revision to Diff 324737.Feb 18 2021, 12:31 PM

Address review feedback.

kzhuravl marked an inline comment as done.Feb 18 2021, 12:33 PM
rampitec added inline comments.Feb 18 2021, 12:42 PM
llvm/test/CodeGen/AMDGPU/elf-header-flags-sramecc.ll
6

Can you add gfx90a and gfx90c?

rampitec added inline comments.Feb 18 2021, 12:44 PM
llvm/test/CodeGen/AMDGPU/elf-header-flags-sramecc.ll
6

Actually only gfx90a.

kzhuravl updated this revision to Diff 324761.Feb 18 2021, 1:51 PM
kzhuravl marked 2 inline comments as done.

Address @rampitec 's comments.

arsenm added inline comments.Mar 3 2021, 6:30 PM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
4546

Why &&? I also have no idea what this type is, so I think the auto hurts

4547

I'm guessing this is an Optional, in which case *HasAbiVer

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
5168

The subtarget still has the register even if xnack isn't enabled

kzhuravl marked 2 inline comments as done.Mar 4 2021, 12:03 PM
kzhuravl added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
5168

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

kzhuravl updated this revision to Diff 328266.Mar 4 2021, 12:06 PM

Rebase and address review feedback.

arsenm accepted this revision.Mar 17 2021, 3:43 PM
This revision is now accepted and ready to land.Mar 17 2021, 3:43 PM
This revision was landed with ongoing or failed builds.Mar 24 2021, 8:54 AM
This revision was automatically updated to reflect the committed changes.
jhenderson added inline comments.Mar 26 2021, 1:01 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
5085

@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
5007–5009

This code appears to be untested.

5013–5014

This code is untested.

5025–5026

This code appears to be untested.

5030–5034

This code is untested.

5045–5046

This code appears to be untested.

5048–5051

This code appears to be untested.

5054–5060

This code is untested.

@kzhuravl, are my comments going to be addressed?

kzhuravl added a comment.EditedApr 26 2021, 6:35 AM

@kzhuravl, are my comments going to be addressed?

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.

@kzhuravl, are my comments going to be addressed?

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.