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

In D43831#1027001, @nhaehnle wrote:

Could you please reduce the testcase?

So, the expcnt(0) is generated because there is a buffer_atomic_add in that test case, which actually makes sense to me. Looks like this patch also fixes that case.

Oh, I thought expcnt() was for parameters export only, but it can be used for GDS instructions as well.

Here's a test case where a expcnt(0) is generated https://hastebin.com/uyuhabeqov.pl

This new revision attaches a LLVM IR testcase that reproduces the issue, not sure if the test can still be reduced compared to the original monster shader.

In D35073#976838, @dstuttard wrote:

Any more comments on this change? If not I'll land this in the next few days.

In D38173#884464, @nhaehnle wrote:

Maybe you could add a helpful comment on isClamp? Either way, LGTM.

v2: - preserve valye by using getNamedOperand()->getImm() instead

• add v_clamp_mac_to_mad test in clamp-modifier.ll
• add v_omod_mac_to_mad test in omod.ll

In D30609#693535, @mehdi_amini wrote:

I'm not sure I understand how this patch makes sense as is.

As of today, you can't express this constraint in LLVM in a fine grain way. It is the responsibility of the Front-end to emit/define intrinsic with the right set of conservative flags in the absence of fine-grain ones.

https://reviews.llvm.org/D26348 is probably better.

Hi there,

Or more generally, avoid sinking all functions declared with the convergent attribute.

Okay, I think the correct fix is to avoid sinking intrinsics if the convergent attribute is set.

Hi James,