There may be changes to some details in the LLVM patch; once they are finalized I will update the Clang patch.

Use the same ELF section name as GCC

In D54487#1325584, @JohnReagan wrote:

I'll ask the larger question of why did you pick ".LLVM.command.line" as the section name? While distasteful, there might be existing scripts that do things like:

readelf -p .GCC.command.line a.out

to dump out the arguments like

String dump of section '.GCC.command.line':

[     0]  a.c
[     4]  -mtune=generic
[    13]  -march=x86-64
[    21]  -O2
[    25]  -frecord-gcc-switches

Yes, the format of contents is not well-defined (as mentioned in the gcc man pages), but for somebody switching from a gcc-based toolchain to an LLVM-based toolchain, would they expect the sections to have the same names?

Your description says this would allow clang to add -frecord-gcc-switches (and not -frecord-llvm-switches) so would someone expect that clang would follow the module and create a '.GCC.command.line' section name?

Or perhaps also create a .GCC.command.line section that would contain 'see .LLVM.command.line for additional command line descriptions' so at least existing scripts might not get tripped up?

Or am I overthinking this from working in a high "upward-compatibility, don't break anything" environment for 30+ years?

I'm not certain who else might want to review this, but I'm adding some who have reviewed the touched files recently.

Rebase, ping

I posted to the commit first by accident; copying here:

I realize now that this is insufficient. The other DI* metadata which have a file: (e.g. DILocalVariable) can point to an arbitrary DIFile, and I don't confirm that it matches the source: of all other files in the same DICompileUnit.

In D53153#1315109, @rjmccall wrote:

Okay. So it's still the case that all symbols will be defined within the linkage unit; it's just that some things might need to get exposed outside of it.

LLVM does provide a dso_local attribute which you could use unconditionally on every symbol without actually changing visibility.

Ping

Ping

Ping

In D53153#1314798, @rjmccall wrote:

You still have the same linkage model for those other languages, right? Ultimately there's something like a kernel function that has to be declared specially and which becomes the only public interface of the code?

The Verifier patch has landed as https://reviews.llvm.org/rL348022

In D53153#1291348, @rjmccall wrote:

In D53153#1289380, @scott.linder wrote:

I don't believe that is currently the case (the unrestricted linking of OCL code to OCL code via a dynamic linker), but we do have the notion of a static link step, followed by dynamic linking at runtime. The static link step is currently via IR, but we plan to support linking object files. Maybe I misunderstand the distinction between linkage and visibility, but it seems reasonable that a user would want to have e.g. a non-kernel function participate in static linking, but not be preemptible in the final shared object. The intention with this patch is to allow this with something like -fvisibility hidden without disrupting kernel symbols, which must appear in the dynsym for the reasons mentioned earlier in the thread.

Okay, this still doesn't need user-controlled symbol visibility. The basic question here is whether there is any need for anything other than kernel functions to have dynamic linkage. If not, then you really just need to mark everything as hidden except for kernels. You *could* do that marking in Sema, but frankly that seems like an implementation detail escaping into the AST, and it's just going to cause you unwanted pain and sub-optimality. (For example: -fvisibility=hidden doesn't actually change the visibility of external declarations for reasons that make a lot of sense for general-purpose environments but probably don't matter to you at all. If you're doing this to get better code-generation for references to external entities that are going to end up within the image, you actually need to add a visibility attribute to every single top-level declaration to get that effect. It's much easier to just do it in a pass on the module.)

The constraint is only per-CU. I've attempted to capture this in the latest patch, although I don't quite understand the interaction of all of the DI* metadata, so I may have still missed cases.

Rebase

Update documentation for new option

In D54487#1298669, @JohnReagan wrote:

Got it. Yes, SHT_NOTEs probably don't survive into the final image so SHT_PROGBITS is what was chosen. I wonder if the linker special cases on the section name to keep it from being part of a load section? Seems like a waste to read that stuff into memory at image activation time.

Rebase

Actually, after thinking about this I agree more with the approach of dropping or somehow warning/failing in the DWARF backend, rather than verifying at the IR level. Conceivably another DI backend which implements embedded source might not have the same restriction as DWARF, so restricting the IR doesn't seem right.

See https://reviews.llvm.org/D54953 for the Verifier work.

LGTM; if there are other regressions we can address them as they come up, but I don't see any major ones now.

Update documentation for new option and error in the driver when generating for unsupported object-file format.

Use unique_ptr for HSAMetadataStreamer

In D53329#1300435, @dblaikie wrote:

Thanks!

So I can see where/how this fails now - the LLVM backend seems to require that if any file has embedded source, they all do. Would you be able to/would you mind adding a debug info verifier check for this? That'd help make this fail sooner. (@scott.linder - perhaps you'd be able to/interested in doing this - looks like you wrote the initial implementation?)

Beyond that, then the question is whether this fix is the right way to satisfy that requirement (I'm assuming the requirement is reasonable - though I've not looked at the embedded source support & have no idea if there's a way/it's reasonable to support some embedded source and some not). My big question & I've tried to do some debugging to better understand this - but don't have the time to dive much further into it (& haven't really figured it out as yet): Why is this source not accessible through this API at this point? is the fallback to the main file always accurate?

(as far as I got was figuring out that SourceManager::getBufferData had an SLoc with isFile == true for the header, but isFile was false for the SLocs related to the .cpp file. Can't say I know why that is/what that means & would like to understand that (or someone else more familiar with this stuff who already knows can review this code) before approving this patch - if you could help explain this, that'd be great)

In D48179#1300366, @kzhuravl wrote:

what is the rationale for putting the metadata verifier in binary format instead of support?

Preserve docs for code object v2 metadata

r346884

r346884

Looking a bit further, it seems some other object-file formats have conventions for naming (e.g. Mach-O sections are _snake_case) and so .LLVM.command.line may not end up being what we want universally.

Change canonical option name to -frecord-command-line and add an alias from -frecord-gcc-switches

Remove new ".commandline" ASM directive in favor of standard ones (e.g. .section, .ascii)

Ping

Sorry for taking so long to look at this. I am running some OCL conformance tests to get an idea of what impact this will have in terms of regressions, but if we believe this is the correct thing to do I don't think that should have any impact on when it is merged anyway.

In D54487#1297520, @probinson wrote:

I don't really see a need for a new directive, it's convenient but does nothing you couldn't do with existing directives.
It does have some downside, in that it's supported only by the LLVM integrated assembler.

In D54489#1297504, @troyj wrote:

I realize that you're probably striving for option compatibility with gcc, but continuing to name it -frecord-gcc-switches when it actually records Clang switches seems weird to me. It almost sounds like something that would dump gcc equivalents of all Clang options, or maybe let you know which Clang options you've used that match gcc options. Either way, by the name -- if you aren't familiar with the gcc option -- it doesn't read like it records Clang options.

Would it be that bad to name it -frecord-clang-switches? Or just -frecord-switches?

In D54487#1297461, @JohnReagan wrote:

I know you are just trying to match gcc behavior, but do you know why they use SHT_PROGBITS instead of something like SHT_NOTE for the information? On OpenVMS, we use .note entries to record compilation date/time, compiler version, command line, etc. information that is later used by our linker to print out detail link maps. Or is the intention that the information get propagated all the way to the final executable or shared library?

Update getAMDGPUNote param name to match its new type

Add missing AMDGPUNote type

The heavy use of templates does lead to some awkward usage, but it can also make some common use-cases very natural. I agree there is probably room to improve, but it is orthogonal to this patch.

Rebase tools/llvm-readobj/ELFDumper.cpp

I don't quite understand what distinguishes the two versions of MCSubtargetInfo (getSTI() vs STM), but it appears a patch from Konstantin earlier this year changed this specific instance from STM.getFeatureBits() to getSTI(). Changing this back LGTM but I don't know what the rationale was in the first place, if it wasn't a typo, so I don't want to sign off without asking.

In D52894#1289265, @efriedma wrote:

Please rebase on top of https://reviews.llvm.org/D52537 .

I don't believe that is currently the case (the unrestricted linking of OCL code to OCL code via a dynamic linker), but we do have the notion of a static link step, followed by dynamic linking at runtime. The static link step is currently via IR, but we plan to support linking object files. Maybe I misunderstand the distinction between linkage and visibility, but it seems reasonable that a user would want to have e.g. a non-kernel function participate in static linking, but not be preemptible in the final shared object. The intention with this patch is to allow this with something like -fvisibility hidden without disrupting kernel symbols, which must appear in the dynsym for the reasons mentioned earlier in the thread.

I still think disambiguating "indirect call" in LLVM is worthwhile, so here is another attempt at addressing feedback.

Ping

Ping

Ping

Remove unused AMDGPUSubtarget::EnableVGPRSpilling; I missed this in the first patch.

@klimek is there anyone I should add to take a look at this? As far as the YAML is concerned I believe this is a non-functional change.

Add lit LABEL for test case

Address feedback

Ping

Ping

Move addPass for new pass

Rebase and update:

Add test for promoting invoke instruction

In D53153#1263882, @rsmith wrote:

In D53153#1263848, @scott.linder wrote:

Beyond constructors/destructors I believe an API which we implement through access to dynamic symbols for global variable is clGetProgramBuildInfo(..., CL_PROGRAM_BUILD_GLOBAL_VARIABLE_TOTAL_SIZE, ...) which is defined as "The total amount of storage, in bytes, used by program variables in the global address space."

Presumably this is looking at the size of a certain section / segment of the image. It's unclear to me why symbol visibility would affect this in any way.

Don't mark namespace-scope global variable declarations in OpenCL with explicit default visibility

Update note type value

In D53445#1271220, @kzhuravl wrote:

In v2 asm, we had the following:

.option.machine_version_major
.option.machine_version_minor
.option.machine_version_stepping

What are the equivalents for v3 asm? The updates you made do not seem to describe those... Unless I missed it.

Update note type value.

Ping