@yaxunl Could you please commit this change on my behalf? I don't have a write access to the trunk.
Thank you

Retry build.

In D99071#3652603, @nickdesaulniers wrote:

In D99071#3652570, @pvellien wrote:

In D99071#3652434, @nickdesaulniers wrote:

Are declarations for @llvm.amdgcn.is.shared supposed to be showing up in my IR, even when my target triple is _not_ amdgcn-amd?

@nickdesaulniers thanks for letting us know about this, I would like to understand more about this. Is it possible for you to share a simple test case (along with which target) and build script to me so that I can reproduce it on my end.

$ cd llvm-project
$ clang -cc1 -internal-isystem llvm/build/lib/clang/15.0.0/include -nostdsysteminc -std=gnu2x -triple x86_64-linux-gnu clang/test/CodeGen/attr-function-return.c -emit-llvm -o - -mfunction-return=thunk-extern -fsanitize=address

In D99071#3652434, @nickdesaulniers wrote:

Are declarations for @llvm.amdgcn.is.shared supposed to be showing up in my IR, even when my target triple is _not_ amdgcn-amd?

In D116216#3251066, @yaxunl wrote:

LGTM. Thanks.

@nikic thanks.

Removed amdgpu-asan-noprintf.cu and added amdgpu-asan-printf.cu testcase.

In D117184#3250460, @nikic wrote:

In D117184#3250457, @pvellien wrote:

@nikic thanks a lot for accepting. Could you please commit this patch on my behalf? I don't have a commit access to llvm. thanks

Sure, can you let me know what Name <email> to use for the commit?

@nikic thanks a lot for accepting. Could you please commit this patch on my behalf? I don't have a commit access to llvm. thanks

Address review comments. thanks

@nikic Thanks a lot for the detailed description to reduce the test-case, it greatly helped me.
Updated the patch with test-cases.

Hi @nikic
https://gist.github.com/pvellien/e4f519d17b25adf10fdd5978ee0b1de9
After trying a lot, this is minimal IR i can able to find. It is reduction from 84k lines to 617 lines.
Please compile with Opt with O3.

hi @nikic I tired both bugpoint and llvm-reduce both of them, failed to reduce the issue. It would be very much helpful to know how to proceed further. I'm new to the llvm middle-end, I could really use some help here.
If it will be helpful I will share the complete IR module which fails at assert.

@nikic generating a small test-case is really hard, since it was triggering due to small peephole under very specific constraints. what would you advise here?

thanks for your reply @nikic
I'm trying to reduce the test-case to a minimal one. It may take some time. I will share as soon as I have it.
If I have to relax the assert, can I check whether the constant expr is a compare instead of true and false const?
Or If I have to use target dependent constant folding, how to do that?

thanks for your reply @nikic
I'm trying to reduce the test-case to a minimal one. It may take some time. I will share as soon as I have it.
If I have to relax the assert, can I check whether the constant expr is a compare instead of true and false const?
Or If I have to use target dependent constant folding, how to do that?

Hi @nikic This patch triggers an assertion in simplifycfg.cpp pass.
https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Transforms/Utils/SimplifyCFG.cpp$5800 this assertion is failing because that CaseConst returned is neither a true or false constant but a compare constant expression.
In my case, the instruction that triggers this assertion is

@lebedev.ri updated with test-cases.

@yaxunl It would be very much helpful to know how to write test coverage for this particular patch? thanks

The testcases related to this patch are already added via this patch https://reviews.llvm.org/D112820.

Thanks for accepting. I don't have commit access to llvm trunk yet. Could you please commit this patch? thanks.

Added a check in amdgpu-asan.cu file

Addressed comments.

rebased

Addressed review comments.

In D110054#3024859, @gandhi21299 wrote:

I can help commit this patch. Can you please rebase your work with the latest main branch and upload the patch again, @pvellien ? Thanks

Rebase on top.

Hi, The failures in the debian builds are not related to this patch changes. I don't have a commit access to llvm trunk, could any of you please land this patch on my behalf. Thanks a lot for your time.

@foad thanks.

Added test.

Hi @yaxunl It will helpful to know what kind of test is expected to test the linkage type of generated init/fini kernels. The related test-cases for init/fini kernels are added in this patch - https://reviews.llvm.org/D105682

In D95232#2572950, @dblaikie wrote:

In D95232#2566553, @MaskRay wrote:

(CC @jhenderson @rnk )

The issues with the const std::string & parameter symbolize* API:

• The LLVMSymbolizer instance needs to construct ObjectFile, which may be a duplicate if the application has an existing ObjectFile instance.
• The file reading error is buried in the API. The application cannot easily tell there is an IO error or incorrect address error and may keep symbolizing more addresses after an IO error. In addition, the user may want to handle the IO error themselves.
• It does magic things (opening an auxiliary file) which may be of security concern for some usage: it constructs a .dsym path for a Mach-O file; it constructs a path from build ID for an ELF file; it constructs a .gnu_debuglink path if the section is present in an ELF file; it constructs a PDB path for a PE file. Personally I'd like the feature to open auxiliary files to be specified explicitly by the user on ELF.

These ^ are the reasons you're suggesting it would be good to refactor the existing code to take an ObjectFile instead of a std::string file name?

Yes

The const ObjectFile & symbolize* does not have the ability to open magic auxiliary files. This is not clear from the API.

This ^ is a limitation of these newly proposed ObjectFile APIs? So they won't be entirely compatible with the existing string-based APIs? So it would be difficult/not possible to refactor the existing code to use these APIs?

I think we can pass the file with debuginfo as an additional parameter as suggested by @MaskRay regarding refactoring all the use-cases within llvm to new APIs I'm not sure how complicated it would be in tools such as symbolizer, objdump etc

In D95232#2567721, @jhenderson wrote:

I'm not sure if this is what @dblaikie was referring to, but it seems like a good way to test new public-facing APIs would be to write gtest unit tests. I don't think there's any precedent for this for the symbolizer API, so it may not be particularly easy. Refactoring llvm-symbolizer to make use of the new API is probably a good idea, and it may help with the test coverage, but there is a risk that a future refactor beyond that will cause llvm-symbolizer to no longer use the API, and therefore the test coverage is lost.

In D95232#2566553, @MaskRay wrote:

(CC @jhenderson @rnk )

The issues with the const std::string & parameter symbolize* API:

• The LLVMSymbolizer instance needs to construct ObjectFile, which may be a duplicate if the application has an existing ObjectFile instance.
• The file reading error is buried in the API. The application cannot easily tell there is an IO error or incorrect address error and may keep symbolizing more addresses after an IO error. In addition, the user may want to handle the IO error themselves.
• It does magic things (opening an auxiliary file) which may be of security concern for some usage: it constructs a .dsym path for a Mach-O file; it constructs a path from build ID for an ELF file; it constructs a .gnu_debuglink path if the section is present in an ELF file; it constructs a PDB path for a PE file. Personally I'd like the feature to open auxiliary files to be specified explicitly by the user on ELF.

The const ObjectFile & symbolize* does not have the ability to open magic auxiliary files. This is not clear from the API.

If we want to keep just one set of symbolize* overloads and go for const ObjectFile &, we perhaps need to store ObjectFile handles instead of StringRef handles for LLVMSymbolizer's internal maps, and make Expected<SymbolizableModule *> LLVMSymbolizer::getOrCreateModuleInfo(const std::string &ModuleName) public.

I want to check with folks whether whether some refactoring is needed in this area.

---

This can be useful for clients who already have a in-memory object files to symbolize for.

I'd hear more about the use case. For in-memory object files, how do you handle .gnu_debuglink and the build ID stuff. It is possible that you don't want to handle this.

In D95232#2566234, @MaskRay wrote:

From my reads of the history of this component, I think DebugInfo/Symbolize was extracted from llvm-symbolizer because sanitizer runtime needs it. Later other LLVM internal tools (sanitizer runtime, sancov, sanstats, llvm-xray, etc) use the API as well.
I don't find usage from open-source projects. There could be, but I speculate that if do some refactoring the friction will be small.

Currently the std::string overloads are mainly used. It seems to me that we don't need to std::string overloads. They can likely all switch to the const ObjectFile & overloads.
I will take a stab at cleaning up the call sites.

@pvellien @scott.linder The concern with not testing the new API is that they are otherwise unused (I guess that you may have downstream projects which may adopt them soon) and may be deleted by other contributors as dead code.
If you have some specific use cases, contributing unittests would probably be a good idea.
I'll try refactoring the API, if const std::string& is replaced with const ObjectFile & overloads, then the API will be used by in-tree code and will be less likely deleted as dead code.

In D95232#2566156, @dblaikie wrote:

In D95232#2565937, @pvellien wrote:

In D95232#2564264, @dblaikie wrote:

Generally there should be test coverage - otherwise these APIs are likely to be cleaned up as unused, even if the chance of actual bugs in them is small (since they're just simple wrappers).

Perhaps some unit testing would be suitable?

Hi dave, thanks for your comments. I was worried about this thing. But there are no existing tools in llvm that work on object file directly expect there is a usage for one existing API - symbolizeCode.

Sorry, I'm not quite following this last sentence, could you rephrase it?

The first part (" But there are no existing tools in llvm that work on object file directly ") doesn't seem quite right, so far as I'm reading it - llvm-symbolizer, the command line tool, can operate on object files and many of the llvm-symbolizer tests operate on object files. (which also makes me wonder about this change in general - llvm-symbolizer can already work on object files, so why are new APIs required, given the functionality already exists/is used/tested?)

I don't know how it is reliable to add unit tests, I would highly appreciate your thoughts on this. thanks

In D95232#2564264, @dblaikie wrote:

Generally there should be test coverage - otherwise these APIs are likely to be cleaned up as unused, even if the chance of actual bugs in them is small (since they're just simple wrappers).

Perhaps some unit testing would be suitable?

ping

Added FIXME

Refactor code

This change is wrong, the different patch is landed in llvm to handle global address space access in gfx60x for HSA Os. So closing it.

Fixed a small typo in comments. Nothing else is changed from prior diff.

Included the suggestions given by tony.

If this is fine, place land. I don't have commit access yet.

added a blank line in amdgpuusage.rst file.

Updated the AMDGPUUsage doc. Also I don't have commit access, please commit this patch on my behalf.
Thanks a lot your time.

Updated the code according to scott's suggestion. And I tried to compile sample program with all possible combinations of processors and ABIs( amdhsa, mesa3d, amdpal), it seems like the backend supports all the ABIs for all processors. So Instead of adding a separate column, (since values of processors will be the same) I added a text pharse above the processor table which documents. Is it right thing to do so?

In D92483#2466144, @scott.linder wrote:

In D92483#2463773, @rampitec wrote:

In D92483#2463314, @pvellien wrote:

In D92483#2458755, @rampitec wrote:

But I still do not think this is a right thing to do to accept amdhsa on SI, even if you turn off flat instructions. SI does not support HSA.

As far as I the support of HSA in SI processors. Tony suggested that gfx60x processors should support AMDHSA OS because it is capable of supporting OpenCL 1.2 which not need generic pointers. This topic was discussed in amdgcn weekly and using MUBUF instructions for global address space in gfx60x is decided. The same is also documented in AMDGPU User Guide earlier.

But it is not. In fact AMDGPUUsage.rst says that SI does NOT support amdhsa and it is only supported starting from gfx700:

**GCN GFX6 (Southern Islands (SI))** [AMD-GCN-GFX6]_
-----------------------------------------------------------------------------------------------------------------------
``gfx600``  - ``tahiti``    ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
                                                                   support
                                                                   generic
                                                                   address
                                                                   space
``gfx601``  - ``pitcairn``  ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
            - ``verde``                                            support
                                                                   generic
                                                                   address
                                                                   space
``gfx602``  - ``hainan``    ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
            - ``oland``                                            support
                                                                   generic
                                                                   address
                                                                   space

It is only amdpal listed here.

What was discussed is that we can use buffer instructions for global on SI even if amdhsa is not supported.

At one point, while you were on vacation, the consensus was "support amdhsa OS with gfx6, and fail to compile in the presence of e.g. generic pointers".

Has there been more discussion since then?

In D92483#2458755, @rampitec wrote:

In D92483#2458659, @rampitec wrote:

In D92483#2458152, @pvellien wrote:

if mcpu is specified, set Gen to the correct generation that cpu belongs to, by initializing it with SOUTHERN_ISLANDS.
if mcpu is not specified (in this case GPU = "generic-hsa"), set Gen to the SEA_LANDS to be in parity with previous code> Comments inline

You are forcing SI for any SPECIFIED GPU instead:

!GPU.contains("generic") ? SOUTHERN_ISLANDS

Read this code line: if CPU is NOT generic, force it to SI.

Ugh. I see what you mean now, it took debugger to understand. The real value of Gen is assigned below in the GCNSubtarget::initializeSubtargetDependencies().

But I still do not think this is a right thing to do to accept amdhsa on SI, even if you turn off flat instructions. SI does not support HSA.

Comments inline

Removed error reporting based on string comparison. Updated the memory legalizer tests to include amdhsa for gfx60x

fixed a typo in comments.

Comments inline

Updated with stanislav comments