Hi there,

FWIW, this fixes a regression that introduced important glitches with Dead Rising 4 with both RADV/AMDVLK. Can this be pushed?

Thanks for the quick fix Matt!

Is this expected that a bunch of tests with Mesa now crash because amdgcn.s.waitcnt is not found?

This change introduced CTS regressions with RADV like dEQP-VK.spirv_assembly.instruction.graphics.float16.arithmetic_3.atan2_frag and friends.
Can you investigate ?

Do you plan to fix it? It breaks a bunch of games.

Yes.

Hi Nicolai,

Hi Nicolai,

Sorry for the noise.
Should be fixed with https://patchwork.freedesktop.org/patch/310419/?series=62097&rev=1

Do we need to update the intrinsic name from mesa?
Btw, it's not related to RADV, RadeonSI is probably affected too because the LLVM backend is common.

This change introduces a bunch of failures with CTS on GFX8/GFX9. Looks like the IR validation fail now, see below:

In D59990#1530603, @alex-t wrote:

In D59990#1530511, @hakzsam wrote:

I have updated the change ttps://reviews.llvm.org/D62614 this Sunday.
The new one takes completely different approach. I'd appreciate very much If you could try it.

D62614 doesn't fix the issue.

Okay. I'm about to start partial revert of the change.
Could you please provide me test cases so that I can check if my further fixes help.

I have updated the change ttps://reviews.llvm.org/D62614 this Sunday.
The new one takes completely different approach. I'd appreciate very much If you could try it.

In D59990#1527275, @hakzsam wrote:

In D59990#1521409, @alex-t wrote:

In D59990#1517891, @hakzsam wrote:

Hi there,

This change introduces a regression with RADV, all dEQP-VK.subgroups.arithmetic.framebuffer.* are failing now.
Can someone look into this?
Thanks!

I have a patch that fixes the issue in another test suite. Could you please suggest how to check if it also fixes RADV?
https://reviews.llvm.org/D62614

This patch fixes the CTS failures on my side. I have just tried the latest version.

In D59990#1521409, @alex-t wrote:

In D59990#1517891, @hakzsam wrote:

Hi there,

This change introduces a regression with RADV, all dEQP-VK.subgroups.arithmetic.framebuffer.* are failing now.
Can someone look into this?
Thanks!

I have a patch that fixes the issue in another test suite. Could you please suggest how to check if it also fixes RADV?
https://reviews.llvm.org/D62614

Hi there,

Thank you too!
Can you eventually push the patch for me? I think I no longer have commit access since the LLVM relicense stuff.

v3: - remove most of the LLVM IR

In D52019#1496159, @arsenm wrote:

In D52019#1496145, @hakzsam wrote:

Hi there,

This change introduces a regression with RADV and Doom 2016, see https://bugs.freedesktop.org/show_bug.cgi?id=110636
We just discovered this, so LLVM 8 and master are affected. :(
I'm trying to find the root cause, I will let you know.

Did r360293 fix it?

Yes, it did fix the problem.
Do you plan to backport to LLVM 8?

Hi there,

In D60846#1494878, @spatel wrote:

In D60846#1494561, @hakzsam wrote:

Hi folks,

This introduces a regression with RADV (the open source Vulkan driver in mesa) and some CTS tests, here's the list of failures:

Is the regression a compiler-crash, program crash, miscompile, or performance loss?
Probably related to addrspace?

Hi folks,

In D61313#1489140, @nhaehnle wrote:

That's an interesting catch. Basically a Write-after-Write hazard.

Though this seems like the wrong place to add this check, since the function is specifically called canMoveInstsAcrossMemOp.

I rather suspect that instead addToListsIfDependent needs to be fixed instead, so that when the scan hits the S_CMP_EQ_U32 it recognizes the WAW hazard and adds it to the instructions to be moved.

I also wonder if this is related to D60459.

In D61313#1490515, @ronlieb wrote:

have you run all the lit tests on this patch ?

In D61313#1489386, @ronlieb wrote:

This sure does look like the same problem to me. https://reviews.llvm.org/D60459

ping?
SI is still broken without this patch.

In D59772#1474229, @arsenm wrote:

In D59772#1473800, @hakzsam wrote:

Hi Matt,

This change introduces a regression with Overwatch and RADV (cf. https://bugs.freedesktop.org/show_bug.cgi?id=110476).

Here's the attached LLVM IR https://pastebin.com/raw/VqRF4unW

Can you have a look?
Thanks!

r358890

Hi Matt,

This fixes the SI regression with RADV.
Thanks a lot Tim.

Hi,
This change breaks SI, at least with RADV.
Here's how to reproduce: ./deqp-vk --deqp-case=dEQP-VK.glsl.builtin.function.integer.bitfieldreverse.ivec3_highp_tess_control

Thanks for the quick fix Matt!

This introduces a regression with this LLVM IR https://hastebin.com/yuzomezazi
It crashes inside llvm::SIInstrInfo::areLoadsFromSameBasePtr for some reasons.

Hi there,
Do you have the cmpswap fix somewhere?
Thanks

Thanks for the fix!

Please have look at https://bugs.llvm.org/show_bug.cgi?id=40129

Unfortunately, I can't get any LLVM IR because it crashes too early in the process. You can also reproduce the problem with RadeonSI btw.

Yes, it does fix the issue. Thanks!

This patch breaks RADV (and probably RadeonSI as well). Here's a backtrace of the problem:

Did you get a chance to look into this?

This change breaks most of the subgroups tests with RADV (ie. dEQP-VK.subgroups.arithmetic.*).

Hi Nicolai,

This regresses the following tests on RADV:

Hi Nicolai,

Thank you for this really important fix!

r340417

r340416

Should I push the patch as is? Or does it need another revision?

v5: rename MAX_COMMON_ADDRESS to MAX_AMDGPU_ADDRESS

In D50974#1206359, @arsenm wrote:

LGTM

Both patches (r340171 and r340172) have been reverted with r340202 because it will be easier for a backport.
This new version squashes these two and includes the alias rules table fix (ie. out of bounds access).

v2: add a test with swapped parameters

In D47261#1205868, @jvesely wrote:

NACK. This patch is clearly wrong.
MAX_COMMON_ADDRESS is used in AMDGPUAAResult::ASAliasRulesTy::getAliasResult to filter indices to the ASAliasRules table which is 6x6. Allowing address space 6 leads to out of bounds access to the array.

This has been reported and answered in https://bugs.llvm.org/show_bug.cgi?id=38113

v3: use static_assert()

v2: add a very simple test for 32-bit addr space

In D47261#1109743, @arsenm wrote:

Needs a test, preferably the full set of AA checks with 32 bit constant

Without this patch, it appears to me that we are selecting
the wrong operand when inverting conditions. In the attached
test, it will select %tmp3 instead of %tmp4. To fix it, just
use 'A' as everywhere.

In D40556#1077975, @alex-t wrote:

Sorry for the delay. Just submitted to trunk.
It was not about the reverse patch only. I had to change tests accordingly.

Any news on the revert? Thanks!

In D43831#1063216, @msearles wrote:

• Change isLoopBottom() to return true in the case of a single basic block loop
• Clear LoopWaitcntDataMap in between functions

In D40556#1062962, @alex-t wrote:

In fact yes. This is the most correct solution. The stuff in
SIFixSGPRCopies.cpp has got to gone.
So it does not make sense to spend efforts on it. Moreover there is no
correct solution at all
unless we implement one more DivergenceAnalysis upon the machine IR.
So, if it's okay I'd prefer to revert.

In D40556#1061436, @alex-t wrote:

It seems like this patch should be reverted. Recently we have no reliable
way to determine the PHI (or whatever else) divergence on the MI level.
The only correct way is to add the DA algorithm that re-computes all the MI
divergence.
Since we're going to remove the SGPRFix stuff as soon as diverence driven
ISel is ready I consider this patch is not necessary.

Hi Mark,

ping?

v3:

• init EnableDS128 in the constructor

v2:

• use a return ternary operator
• fix feature format
• add small test load-local-f32-no-ds128.ll

Hi guys,

v8:

• add a sanity check when a block is looping over itself
• cosmetic changes

v7:

• replace the LLVM IR testcase with a MIR one
• remove one unused variable in isLoopBottom()
• do not rely on block numbfer in isLoopBottom()

v6: fix the multiple back-edges case and update the testcase

In D43831#1028208, @hakzsam wrote:

In D43831#1028165, @nhaehnle wrote:

In D43831#1028150, @hakzsam wrote:

In D43831#1028129, @nhaehnle wrote:

I haven't yet had the time to fully grok what's going on here, but I suspect this code is fundamentally broken because the whole assumption of having a single loopBottom is wrong. It happens to be true for structured loops with non-uniform control flow, but when there's uniform control flow, there may well be multiple back-edges. What happens in those cases?

I'm not sure either if the current code correctly supports multiple back-edges. I think if there is more than one, loopBottom() will select the basic block with the highest number if it's a successor of the header loop.

The other way around: it will select the basic block with the highest number which has the loop header as its successor.

The question is whether that ends up doing the right thing in all cases.

https://hastebin.com/joqihayohe One case that doesn't work (ie. a vmcnt(0) is missing).

In D43831#1028165, @nhaehnle wrote:

In D43831#1028150, @hakzsam wrote:

In D43831#1028129, @nhaehnle wrote:

I haven't yet had the time to fully grok what's going on here, but I suspect this code is fundamentally broken because the whole assumption of having a single loopBottom is wrong. It happens to be true for structured loops with non-uniform control flow, but when there's uniform control flow, there may well be multiple back-edges. What happens in those cases?

I'm not sure either if the current code correctly supports multiple back-edges. I think if there is more than one, loopBottom() will select the basic block with the highest number if it's a successor of the header loop.

The other way around: it will select the basic block with the highest number which has the loop header as its successor.

The question is whether that ends up doing the right thing in all cases.

v5: run -dce to remove redundant phis

In D43831#1028165, @nhaehnle wrote:

In D43831#1028150, @hakzsam wrote:

In D43831#1028129, @nhaehnle wrote:

I haven't yet had the time to fully grok what's going on here, but I suspect this code is fundamentally broken because the whole assumption of having a single loopBottom is wrong. It happens to be true for structured loops with non-uniform control flow, but when there's uniform control flow, there may well be multiple back-edges. What happens in those cases?

I'm not sure either if the current code correctly supports multiple back-edges. I think if there is more than one, loopBottom() will select the basic block with the highest number if it's a successor of the header loop.

The other way around: it will select the basic block with the highest number which has the loop header as its successor.

The question is whether that ends up doing the right thing in all cases.

In D43831#1028129, @nhaehnle wrote:

I haven't yet had the time to fully grok what's going on here, but I suspect this code is fundamentally broken because the whole assumption of having a single loopBottom is wrong. It happens to be true for structured loops with non-uniform control flow, but when there's uniform control flow, there may well be multiple back-edges. What happens in those cases?

v4: - run 'opt -S -deadarghaX0r -strip -strip-debug -strip-dead-prototypes -instnamer'

• remove RFC on the subject

In D43831#1027405, @msearles wrote:

For the testcase, would you also run -instnamer to rename instances of %<number>? You may want to combine it with other clean-up options, something like this: opt -S -deadarghaX0r -strip -strip-debug -strip-dead-prototypes -instnamer

v3: reduced testcase