LGTM then, but like I said I'm not very familiar with GlobalISel so more eyes might be good

In D58400#1407115, @arsenm wrote:

In D58400#1405973, @scott.linder wrote:

Does anyone have an opinion on returning negative branch targets (e.g. <keep_symbol+0xfffffffffffe0018>)? I don't know how this would ever come up in hardware anyway, or what the hardware would do, but it doesn't seem very helpful in the disassembly.

The hardware doesn't know about the symbol? Do you mean for overflow?

Does anyone have an opinion on returning negative branch targets (e.g. <keep_symbol+0xfffffffffffe0018>)? I don't know how this would ever come up in hardware anyway, or what the hardware would do, but it doesn't seem very helpful in the disassembly.

Improved error message.

Address feedback. Sign-extend all s_branch immediates and change the assembler syntax to represent these as true negative numbers.

Ping

Ping

I think MIR is the best place to look; this now tests that the right pseudo is produced and that spills don't interleave the terminators.

I agree the test does not test much at all, it was just the minimum I could think of considering none of our existing tests notice the change. I'm not sure what pass you mean by 'isel', but 'stop-after=amdgpu-isel' is too early to see the SI_INDIRECT_SRC_* psuedo expanded.

In D57027#1383432, @tpr wrote:

In D57027#1381468, @scott.linder wrote:

The code looks good to me, but I'm wondering why the move from doing this in AsmPrinter::EmitEndOfAsmFile to TargetStreamer::finish? It seems like it is fine to emit directives in either, but I don't understand why the change here.

The idea was so that a disassembler disassembling into a TargetStreamer would go through the same code to emit the directives.

LGTM

See https://reviews.llvm.org/D56871

The code looks good to me, but I'm wondering why the move from doing this in AsmPrinter::EmitEndOfAsmFile to TargetStreamer::finish? It seems like it is fine to emit directives in either, but I don't understand why the change here.

Add test for tail call through GOT

LGTM, pending parents of course.

Thank you for updating this! I only have one small suggestion, but otherwise it LGTM.

LGTM, sorry for the delay in reviewing this.

Address feedback

I've tried to implement carrying a copy of the function global through the DAG from the beginning, rather than recovering it after legalizing the GlobalAddress. I don't know if this is a reasonable approach, but I figured I would post something and see what you think.

Ping

Ping

Update option name

No worries, I agree that we don't gain much with a shorter flag here; explicit seems preferable.

Update option name

Update the -cc1 option name and docs; also update the LangOpt and docs.

Add missing flag to tests

Remove driver options

That sounds reasonable to me. I had already posted a patch with the Driver options, but I will update it to only include the -cc1 version.

Will be superseded by either https://reviews.llvm.org/D53153 or https://reviews.llvm.org/D56871

Rebase and ping

I think we need the option to be in the driver, not just -cc1. We are moving towards the clang driver being the authority on how we do offline compilation for AMDGPU, and hiding this in cc1 doesn't help us.

Rebase and extend tests

Ping

It sounds like -fextern-visiblity= isn't something we really want, then? I think it would have to be implemented in the abstract linkage computer.

In D53329#1356381, @yonghong-song wrote:

@dblaikie @scott.linder Have you got time to look at this issue? Looks like a trivial test case will be able to reproduce the problem. I have the simple example in the previous comments. Practically, -gdwarf-5 -gembed-source is broken now.

In D53153#1355718, @rjmccall wrote:

In D53153#1353256, @scott.linder wrote:

In D53153#1317977, @rjmccall wrote:

I think -fvisibility=hidden isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works. But after this discussion, I'm prepared to accept that (1) we should have some sort of single-image compiler mode that implies that all symbols are defined within the image and (2) you can make your toolchain imply that together with -fvisibility=hidden and then have specific symbols opt in to non-hidden visibility if they need to be accessible to whatever loader / runtime you have.

It seems that explicit visibility attributes on external symbol references (e.g. __attribute__((visibility("hidden"))) extern int foo;) are respected in Clang, so I don't understand the rationale for global visibility controls not applying to them as well. Can you describe why this is the case?

Well, one answer is that that's the behavior that GCC defined when they added global visibility controls, and it would be unreasonable for Clang to deviate in such a fundamental way. However, we do deviate in some other ways in our interpretation of visibility attributes, so that's not a totally satisfactory answer.

The stronger answer is that GCC has a very good reason for this behavior. Global visibility controls apply to all code in the translation unit. While programmers often mentally distinguish between "project" headers (which they control) and "library" headers (which they do not), that is not a difference that is known to the compiler. Therefore, if global visibility controls applied to external symbols, the compiler would end up assuming that all external symbols not annotated as default are provided by the current image. That assumption would have been incompatible with essentially all existing headers for dynamic libraries on GCC's core targets, including system headers; in other words, it would have caused ubiquitous miscompiles.* It would be completely unreasonable to expect all those headers to be changed just for the benefit of a single new compiler feature. Therefore, global visibility controls do not apply to external symbols because doing so would make it impossible to actually enable global visibility controls.

• GCC's rule helps a great deal even in C++, which is generally more sensitive to global visibility: you can typically use an unannotated C++ header under -fvisibility=hidden and have no problems unless you start using its types as exceptions (and GCC tries to work around that, too) or comparing the addresses of inline functions or template instantiations.
  
  John.

In D53153#1317977, @rjmccall wrote:

I think -fvisibility=hidden isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works. But after this discussion, I'm prepared to accept that (1) we should have some sort of single-image compiler mode that implies that all symbols are defined within the image and (2) you can make your toolchain imply that together with -fvisibility=hidden and then have specific symbols opt in to non-hidden visibility if they need to be accessible to whatever loader / runtime you have.

Rebase and ping

rL349155

In D54489#1330255, @rjmccall wrote:

So we're using the same command line option as GCC to produce something in the same section as GCC but formatting that section incompatibly with GCC? That combination of choices does not seem like a good idea.

Update documentation to match section naming change, and to explicitly note that the section format differs from the equivalent GCC format.

Address feedback. The verifier appears to bail on the first failure, at least with llc/llvm-as.

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: