LGTM

LGTM

With this patch you would set features on an address-taken function and ignore whole call stack below it. I.e. its own callees will not be processed. I think you need to continue traversal, just skip actual setting of attributes on such a function. Setting these attributes on a functions it may call in turn shall be fine.

LGTM. Make sure to rerun tests before push as a lot of tests are affected.

LGTM

LGTM

In D94020#2477666, @arsenm wrote:

Should the disassembler still understand it?

Generally speaking it is deprecated but it is supported. Well, not in gfx1030.

Added run line to the unaligned-load-store.ll.

In D93669#2468871, @rampitec wrote:

In D93669#2468868, @arsenm wrote:

In D93669#2468858, @rampitec wrote:

In D93669#2468820, @arsenm wrote:

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

Which tests do you mean? As far as I understand this mostly affects GlobalISel and child patch D93670 contains the actual test, all unaligned cases.

This should cover both. I mean unaligned-load-store.ll (and some others, it's annoying how the test names for these aren't consistent and there is redundancy between files)

It should not change without flat scratch enabled?

In D93669#2468868, @arsenm wrote:

In D93669#2468858, @rampitec wrote:

In D93669#2468820, @arsenm wrote:

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

Which tests do you mean? As far as I understand this mostly affects GlobalISel and child patch D93670 contains the actual test, all unaligned cases.

This should cover both. I mean unaligned-load-store.ll (and some others, it's annoying how the test names for these aren't consistent and there is redundancy between files)

In D93669#2468820, @arsenm wrote:

I think there's some missing tests, these test changes look incidental. Can you add some checks to the existing unaligned load/store base tests

LGTM, but please check with Tony for the documentation.

In D92483#2468598, @t-tye wrote:

I like what Scott describes as it seems intuitively obvious and cleaner. Are there any concerns for making these changes?

LGTM

In D92483#2466967, @t-tye wrote:

In D92483#2466961, @rampitec wrote:

In D92483#2466959, @t-tye wrote:

We did discuss this further and decided that gfx6* should report it supports amdhsa with the restriction that it does not support generic addresses. AMDGPUUsage already notes the restriction in all relevant sections. The LIT tests would also need updating accordingly. The COMgr tests would also need updating.

AMDGPUUsage does not list amdhsa for all 3 SI targets, yet it is going to be accepted with this patch. If that is so then amdhsa has to be listed in the table for these targets.

The table is indicating what runtimes support which ABIs. Since ROCm does not support gfx6* it would still be correct to not list it in the table. It may be useful to have another column to indicate which ABIs are supported by which processors.

In D92483#2466959, @t-tye wrote:

I updated AMDGPUUsage to reflect the current state which is that gfx6* does not support amdhsa. But if this patch allows gfx6* to be supported without generic addresses then that table should also be updated as part of the patch.

LGTM

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#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.

LGTM, but please fix typo in the commit message before submit: "unniform".

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.

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

LGTM

Needs test in Transforms/JumpThreading.
Typo in the commit message "unniform".

LGTM, thanks!

Removed CHAR_WIDTH.

LGTM

Added 0=unlimited to the option description.

ping.

LGTM. Not sure why MODE was also defined by set_gpr_idx, but that can be corrected later if needed.

In D91048#2438775, @kerbowa wrote:

In D91048#2438757, @rampitec wrote:

JBTW, having a need to def M0 probably defeats the idea of potential rescheduling which justifies the separation of the indirect access methods.

The GPR_IDX variants are where we are speaking of adding the def M0 right? These are not expanded until after the scheduler. The MOVREL version's M0 def is expanded earlier during ISel finalization.

My thinking was that we wanted rescheduling the M0 def with the MOVREL version but not the GPR_IDX version and that combining these methods wouldn't allow that.

JBTW, having a need to def M0 probably defeats the idea of potential rescheduling which justifies the separation of the indirect access methods.

I think I've seen similar cases before. LGTM, but will be nice if @qcolombet will look at this too.

In D91487#2421897, @MJDSys wrote:

In D91487#2397822, @rampitec wrote:

Needs test.

Hi @rampitec,

I've tried writing a test for this change, but I've been having trouble (I'm new to LLVM so I might be missing the obvious answer). From what I can see, I thought the correct option would be to create a MIR test, using the same instruction that I saw when investigating the original problem. I started by copying and modifying an existing test in fix-sgpr-copies.mir:

In D92059#2438262, @jonpa wrote:

@rampitec : It would be nice to see this go in soon...

Updated option description.
Updated callback comment to clarify this is used by SLP only now.
Dropped BE test.
Addded a store to the general test to make it useful if anyone needs to run AMDGPU BE on it too.