We should update the v2 and v3 metadata portions of docs/AMDGPUUsage.rst, see https://llvm.org/docs/AMDGPUUsage.html#code-object-metadata

The undefined references in the linker are OK. The user has to provide an external definition, or link against one. I don't think there is anything to do about this in LLVM.

Richard Smith on cfe-dev pointed out some cases where this patch is incorrect, stemming from trying to calculate the linkage too early; the warning will either have to work without the use of isExternallyVisible or will have to be emitted later. This was reverted in r359858 for now.

Use distinct sema diagnostic and do not return early.

@rjmccall I'm not sure if this is the right place to continue discussing this, but I don't have a patch I am happy with and I would rather not post something half-baked.

Clean up use of auto

@rjmccall Would you expect similar conflicts in explicit visibility to result in diagnostics? For example, marking a static variable with an explicit visibility attribute doesn't warn, instead the explicit visibility attribute is silently ignored. GCC 7.3 complains with warning: ‘__visibility__’ attribute ignored [-Wattributes]

In D60967#1476226, @rjmccall wrote:

I suspect that other OpenCL and CUDA implementations don't care at all about symbol visibility for device-side code generation, and giving kernel functions default visibility seems like the right thing to do for the (relatively few) things at the AST level that are sensitive to that, like template visibility. Would you mind reaching out to other implementors about that?

This patch seems fine to me regardless.

In D60967#1476069, @rjmccall wrote:

In D60967#1476057, @scott.linder wrote:

In D60967#1476029, @rjmccall wrote:

Shouldn't it be an error if the user tries to give it hidden visibility?

We effectively consider the user explicitly specifying that a symbol is e.g. a kernel to also carry with it visibility information. We don't want to require the user to redundantly specify that a kernel is not hidden, when it is never meaningful for it to be hidden.

I understand, but if the user explicitly gives it hidden visibility, you should still diagnose that.

Also, shouldn't you just handle this by treating the kernel attribute as a source of explicit visibility at the Sema/AST level?

In D60967#1476029, @rjmccall wrote:

Shouldn't it be an error if the user tries to give it hidden visibility?

In D60967#1475925, @rjmccall wrote:

It seems reasonable to me for target hooks to run after global hooks, but can I ask why AMDGPU specifically relies on this?

r358992

In D60682#1470258, @nhaehnle wrote:

Mesa uses +DumpCode as well. Do we have a good solution for generation both ELF and textual output?

Add a test to confirm split-dwarf is supported for the amdhsa OS in the driver.

Ping

LGTM, thanks! One small change to a comment.

LGTM, thanks! I think you can also drop "provides a method for use by LLPC to write metadata into LLVM IR" from the commit message.

In D58400#1412667, @arsenm wrote:

I'm not sure how the question is whether signed integers are supported. The instruction does treat the offset as signed

After digging a bit more into how we parse/print operands for sopp_br I think there are some more fundamental decisions to make beyond just "do we support signed integers", so I want to avoid changing anything in this patch. I will revisit how we treat them in the future and make any breaking changes all at once, rather than spreading them out.

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