Update other toolchain tests I had missed, and address feedback.

In D70424#1759379, @t-tye wrote:

@scott.linder can answer about the -g question, but I would expect that the CFI is capable of describing the address of the CFA regardless of whether there is a frame pointer by simply knowing the constant offset from the stack pointer.

For AMDGPU it seems to me what we really have is an FP and we optimize away the SP since the stack grows low address to high address, and S32 points to the base of the frame, and not the top of the stack.

Should be fixed in 7ad3636c30a

Yes, I can post a patch, I didn't consider the path at all. I think the test just can't use update_llc_test_checks.

Ping

Ping

Same patch, this time with a working test.

After reading again I think I understand the ask now. Is this closer to what you had in mind?

Actually rebase this time

Rebase and ping

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