This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Restructure code object metadata creation
ClosedPublic

Authored by kzhuravl on Feb 14 2017, 9:31 AM.

Details

Summary
  • Rename runtime metadata -> code object metadata
  • Attempt to fix bug 31610
    • Fixes when assembling
    • Fixes when emitting optional key-value pairs
  • Make metadata not flow
    • Easier to read
  • Switch enums to use ScalarEnumerationTraits
    • Easier to read/understand
    • Do not have to memorize numbers which might change
  • Cleanup and move AMDGPUCodeObjectMetadata.h to AMDGPU/MCTargetDesc
    • Drop "amd." prefix from metadata keys
      • Easier to read
      • Saving on space
  • Introduce in-memory representation for attributes
  • Code object metadata streamer
    • Create metadata for isa and printf during EmitStartOfAsmFile
    • Create metadata for kernel during EmitFunctionBodyStart
    • Finalize and emit metadata to .note during EmitEndOfAsmFile
    • Other minor improvements/bug fixes

Diff Detail

Repository
rL LLVM

Event Timeline

kzhuravl created this revision.Feb 14 2017, 9:31 AM

I am planning to add doxygen comments in a separate patch (tied up a bit at the moment).

kzhuravl edited the summary of this revision. (Show Details)Feb 14 2017, 9:33 AM
arsenm added inline comments.Feb 14 2017, 10:16 AM
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
277 ↗(On Diff #88387)

Should be StringRef?

283 ↗(On Diff #88387)

Ditto

lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.cpp
41–50 ↗(On Diff #88387)

Normally these are static and anonymous namespaces not used.

154 ↗(On Diff #88387)

Braces

172–173 ↗(On Diff #88387)

I think private should be explicitly handled and default to an error value

279 ↗(On Diff #88387)

Early return to reduce indentation

lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h
1 ↗(On Diff #88387)

Missing c++ mode comment

15 ↗(On Diff #88387)

You shouldn't need to include stdint directly. You might want llvm/Support/DataTypes.h instead of you need it

36 ↗(On Diff #88387)

Isn't this limited to just 3? std::array?

40 ↗(On Diff #88387)

StringRef

42 ↗(On Diff #88387)

StringRef

yaxunl added inline comments.Feb 14 2017, 10:19 AM
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
39 ↗(On Diff #88387)

This header file is shared between runtime and compiler. It may not be proper to use namespace llvm here.

Also, changes to this file requires corresponding changes in runtime. Are the runtime changes in place?

lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.h
57 ↗(On Diff #88387)

should the name be EmitEndOfRuntimeMetadata?

kzhuravl added inline comments.Feb 14 2017, 2:41 PM
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
39 ↗(On Diff #88387)

I will probably switch away from llvm namespace here. I will update opencl runtime during the merge if needed.

kzhuravl planned changes to this revision.Feb 23 2017, 3:32 PM
kzhuravl updated this revision to Diff 90728.Mar 6 2017, 11:50 AM
kzhuravl marked 14 inline comments as done.
kzhuravl edited the summary of this revision. (Show Details)

See summary.

kzhuravl added inline comments.Mar 6 2017, 11:52 AM
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h
39 ↗(On Diff #88387)

We have llvm::AMDGPU namespace already. Having ::AMDGPU is causing problems... I thought about switching ::AMDGPU to AMD::AMDGPU, but not sure. I think having llvm::AMDGPU here is fine - it kind of tells that this header file originated and maintained by llvm?

277 ↗(On Diff #88387)

I do not think we should use StringRef here. We want to share this header file with other components (ocl runtime, hcc runtime, offline tools), which might not have StringRef.

283 ↗(On Diff #88387)

Same as above.

lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h
36 ↗(On Diff #88387)

Yes, but won't work with yaml.

Is it possible to isolate the changes that fix bug 31610 and put them in a separate patch? We will need to bug port the fix to the 4.0 branch, and it would be nice not to have to backport such a large patch.

Is it possible to isolate the changes that fix bug 31610 and put them in a separate patch? We will need to bug port the fix to the 4.0 branch, and it would be nice not to have to backport such a large patch.

Ok, I can do that.

yaxunl edited edge metadata.Mar 6 2017, 12:18 PM

Can you add Nikolay as reviewer since changes to the shared header file will affect him. The header file is not solely controlled by compiler.

One way to put things in that header file in llvm name space without changing the header file itself is

namespace llvm {
#include "that_header"
}

Does that work for you?

kzhuravl updated this revision to Diff 91798.Mar 14 2017, 5:13 PM
kzhuravl retitled this revision from [AMDGPU] Restructure runtime metadata creation to [AMDGPU] Restructure code object metadata creation.
kzhuravl edited the summary of this revision. (Show Details)

Updates based on the discussions from last week:

  • Runtime metadata -> code object metadata
  • Do not expose the header file
kzhuravl edited the summary of this revision. (Show Details)Mar 14 2017, 5:14 PM

Can you add Nikolay as reviewer since changes to the shared header file will affect him. The header file is not solely controlled by compiler.

One way to put things in that header file in llvm name space without changing the header file itself is

namespace llvm {
#include "that_header"
}

Does that work for you?

As discussed last week, we are not exposing this header, so llvm namespace sounds ok.

Is it possible to isolate the changes that fix bug 31610 and put them in a separate patch? We will need to bug port the fix to the 4.0 branch, and it would be nice not to have to backport such a large patch.

Ok, I can do that.

Do any of your latest patches contain the bug fix, or are you still working on splitting that out?

kzhuravl updated this revision to Diff 92627.Mar 22 2017, 6:35 AM

Updates based on integrating this change into other components:

  • AccessQualifier::None -> AccessQualifier::Default (there were collisions)
  • Do not use StringRef and ErrorOr in code object metadata header file
kzhuravl added a comment.EditedMar 22 2017, 6:37 AM

Is it possible to isolate the changes that fix bug 31610 and put them in a separate patch? We will need to bug port the fix to the 4.0 branch, and it would be nice not to have to backport such a large patch.

Ok, I can do that.

Do any of your latest patches contain the bug fix, or are you still working on splitting that out?

Is the bug still reproducible? I tried using debian and fedora:rawhide docker images, and could not reproduce those.

fedora:rawhide gcc version: gcc (GCC) 7.0.1 20170309 (Red Hat 7.0.1-0.12)
debian gcc version: gcc (Debian 4.9.2-10) 4.9.2

Is the bug still reproducible? I tried using debian and fedora:rawhide docker images, and could not reproduce those.

fedora:rawhide gcc version: gcc (GCC) 7.0.1 20170309 (Red Hat 7.0.1-0.12)
debian gcc version: gcc (Debian 4.9.2-10) 4.9.2

I can still reproduce 31610 on Arch Linux (gcc 6.3.1) with the current code from trunk.

Is the bug still reproducible? I tried using debian and fedora:rawhide docker images, and could not reproduce those.

fedora:rawhide gcc version: gcc (GCC) 7.0.1 20170309 (Red Hat 7.0.1-0.12)
debian gcc version: gcc (Debian 4.9.2-10) 4.9.2

I can still reproduce 31610 on Arch Linux (gcc 6.3.1) with the current code from trunk.

Can you suggest an appropriate docker image I could use to try reproduce it?

Is the bug still reproducible? I tried using debian and fedora:rawhide docker images, and could not reproduce those.

fedora:rawhide gcc version: gcc (GCC) 7.0.1 20170309 (Red Hat 7.0.1-0.12)
debian gcc version: gcc (Debian 4.9.2-10) 4.9.2

I can still reproduce 31610 on Arch Linux (gcc 6.3.1) with the current code from trunk.

Which revision of LLVM did you test with?

Is the bug still reproducible? I tried using debian and fedora:rawhide docker images, and could not reproduce those.

fedora:rawhide gcc version: gcc (GCC) 7.0.1 20170309 (Red Hat 7.0.1-0.12)
debian gcc version: gcc (Debian 4.9.2-10) 4.9.2

I can still reproduce 31610 on Arch Linux (gcc 6.3.1) with the current code from trunk.

Which revision of LLVM did you test with?

I tested with rL298454.

Can you suggest an appropriate docker image I could use to try reproduce it?

I'm afraid I can't recommend anything specifically, but probably any clean and reasonably up-to-date image should do (I always build in a clean chroot).

Is the bug still reproducible? I tried using debian and fedora:rawhide docker images, and could not reproduce those.

fedora:rawhide gcc version: gcc (GCC) 7.0.1 20170309 (Red Hat 7.0.1-0.12)
debian gcc version: gcc (Debian 4.9.2-10) 4.9.2

I can still reproduce 31610 on Arch Linux (gcc 6.3.1) with the current code from trunk.

Which revision of LLVM did you test with?

r298513

By the way, what could probably be somewhat helpful, are the PKGBUILD file that I use for building the LLVM/Clang packages, and the build script (it runs every 6 hours, providing binary packages for those users of Arch's AUR for whom the build is too heavy):

Of specific interest might be the build() function in the PKGBUILD, in case I'm doing something uncommon (though I believe this shouldn't be the case).

I'm afraid I can't recommend anything specifically, but probably any clean and reasonably up-to-date image should do (I always build in a clean chroot).

Tried base/archlinux:latest with gcc (GCC) 6.3.1 20170306 -- Cannot reproduce, all tests are passing.

Of specific interest might be the build() function in the PKGBUILD, in case I'm doing something uncommon (though I believe this shouldn't be the case).

Trying out this one right now.

By the way, what could probably be somewhat helpful, are the PKGBUILD file that I use for building the LLVM/Clang packages, and the build script (it runs every 6 hours, providing binary packages for those users of Arch's AUR for whom the build is too heavy):

Of specific interest might be the build() function in the PKGBUILD, in case I'm doing something uncommon (though I believe this shouldn't be the case).

https://reviews.llvm.org/D31258

By the way, what could probably be somewhat helpful, are the PKGBUILD file that I use for building the LLVM/Clang packages, and the build script (it runs every 6 hours, providing binary packages for those users of Arch's AUR for whom the build is too heavy):

Of specific interest might be the build() function in the PKGBUILD, in case I'm doing something uncommon (though I believe this shouldn't be the case).

https://reviews.llvm.org/D31258

Thanks, I'll test it right away.

By the way, what could probably be somewhat helpful, are the PKGBUILD file that I use for building the LLVM/Clang packages, and the build script (it runs every 6 hours, providing binary packages for those users of Arch's AUR for whom the build is too heavy):

Of specific interest might be the build() function in the PKGBUILD, in case I'm doing something uncommon (though I believe this shouldn't be the case).

https://reviews.llvm.org/D31258

Thanks, I'll test it right away.

Thanks. Can you let me know how it goes? I would like to get the bugfix and this change in today.

Thanks, I'll test it right away.

Thanks. Can you let me know how it goes? I would like to get the bugfix and this change in today.

Of course. It's almost ready.

This revision is now accepted and ready to land.Mar 22 2017, 3:35 PM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/MC/AMDGPU/runtime-metadata-1.s