- 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
- Drop "amd." prefix from metadata keys
- 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
Details
Diff Detail
Event Timeline
I am planning to add doxygen comments in a separate patch (tied up a bit at the moment).
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h | ||
---|---|---|
0 | Should be StringRef? | |
0 | 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 |
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h | ||
---|---|---|
1 | 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 | ||
56 | should the name be EmitEndOfRuntimeMetadata? |
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h | ||
---|---|---|
1 | I will probably switch away from llvm namespace here. I will update opencl runtime during the merge if needed. |
lib/Target/AMDGPU/AMDGPURuntimeMetadata.h | ||
---|---|---|
0 | 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. | |
0 | Same as above. | |
1 | 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? | |
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.
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?
Updates based on the discussions from last week:
- Runtime metadata -> code object metadata
- Do not expose the header file
As discussed last week, we are not exposing this header, so llvm namespace sounds ok.
Do any of your latest patches contain the bug fix, or are you still working on splitting that out?
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
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.
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).
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.
Thanks. Can you let me know how it goes? I would like to get the bugfix and this change in today.
should the name be EmitEndOfRuntimeMetadata?