In D126389#3546536, @jpages wrote:

In D126389#3540486, @rampitec wrote:

In D126389#3540098, @jpages wrote:

In D126389#3539881, @foad wrote:

In D126389#3538346, @rampitec wrote:

Any performance numbers? The 8 element case was driven by a specific customer program and the performance of the cmp/select was better than movrel.

I don't know why that would be. Maybe the performance characteristics are different on GFX10+ compared to GFX9.

Also on GFX10+ sgpr usage does not affect occupancy, so perhaps the heuristic could be tweaked to make it more likely to use s_movrel (not v_movrel) on GFX10+.

I will try to get some performance numbers on specific games. Do you know if this performance problem was specific to an architecture?
Like Jay said, I could tweak the heuristic for this and only generate it for GFX10+.

All the tests were done on gfx9 flavors. It is also worth noting gfx9 does not have movrel, but uses s_set_gpr_idx_on instead, which may be the difference.

I.e. it may be linked to FeatureMovrel.

I measured the performance with/without the patch on the original game in which we spotted this issue.
It's a small improvement of about 0.5% in the current version compared to without the patch. This was measured on GFX1030.

It seems to be a little bit faster with the patch, I can't really test on gfx9 with this game.

If it's better to keep the initial version for gfx9, I could:

• Only do this patch for GFX10+
• Some flag?

Just verified this patch fixes the rendering corruption caused by D114643 - thanks!

I am strongly in favour of this change. On our target, the sgpr improvements that manifest themselves in reduced sgpr spilling outweighs the code size increase.

Looks good to me - the extra s_cselect's generated are worth the complexity arising from this patch.

The biggest headache comes from the fact that during moveToVALU when S_CMP gets converted to V_CMP the users of SCC need to be handled properly otherwise you end up with a weird copy from SCC. I think this is handled right now by adding an extra copy from VCC to SCC to make the connection between V_CMP and S_CSELECT until it is time for the handling of S_CSELECT. This gets more tricky when there are more uses of SCC I guess.

In D124232#3477435, @sebastian-ne wrote:

I wanted to add this combine before, but I don’t think there is a way to add d16 to an instruction without potentially breaking the code.
The reason is, when an image_sample has the d16 flag enabled, it will use f32→f16 truncation or i32→i16 truncation, depending on the texture format in the descriptor.

Combining image_sample+fptrunc to image_sample d16 works fine for float textures, but I assume we don’t know at compile time if a texture is an integer or float texture.
The application may interpret stored values as float and does an fptrunc, but the texture is actually defined as an integer texture, so the hardware uses an integer trunc instead, giving different results.

LGTM

I'd love this to go in, but when I added the hasOneUse() check it was certainly needed. If my old notes serve me well there was a crash in ctlz.ll test and I concluded this check was needed to avoid some shenanigans in the si-fix-sgpr-copies. I need to double check if the issue has been fixed or is just hidden.

In D115747#3400018, @bsaleil wrote:

@piotr I ran compile time testing and the patch has no significant impact. Worst case is 1.1% and is the only one above 1%. Average is below 0.1%.

Do you have any data on the compile time impact?

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

In D119006#3302156, @foad wrote:

In D119006#3301526, @piotr wrote:

So that indicates an improvement in the average vgpr count.

Yes.

Btw, does the patch result in a noticeably smaller number of merges on average?

No, to my surprise there was absolutely no difference in the amount of merging in any of the 10,000 shaders. I checked by diffing the instruction mix for each shader. The only differences were in VALU and SALU instructions.

So that indicates an improvement in the average vgpr count.

LGTM

LGTM

LGTM

Remove references to "waw" as the issue can also trigger in other scenarios, as Jay pointed out to me (thanks).

In our usual compile-time tests it shows 0.056% degradation on average, worst case 0.9%.

For the record, D113005 indeed fixes the CTS issue I was seeing with D108830. Thanks.

Ping.

LGTM.

Tests pre-committed in 7457fe3dd44a0bc4b0296149c48188accefda5fa.

Added -global-isel-abort=0 and restored the tests.

Sounds good to me. The runtime improvement from clustering is notoriously difficult to assess, but your static data shows some potential benefit.

I was thinking about this, but not sure what the test would demonstrate, as the patch just limits the number of cases for which this pass works. With this patch, there are no test changes in the existing set of tests, as expected.

The patch looks good to me (modulo the lint warnings).

The change makes sense to me in general.

In D109159#2986876, @hliao wrote:

In D109159#2986862, @foad wrote:

Just for the record, see also this comment from D82370:

Additionally, remove pattern for selects with 64-bit
inputs, which are rare, because handling them properly
requires more thought.

This reverted part of D81925, which originally added both 32- and 64-bit patterns.

In fact, we found that 64-bit select is not rare, partially due to the use of 64-bit pointers (tend to be uniform in most cases.) This patch is developed to address that pending issue in D81925.

Add test.

Test case added.

Yes, based on Matt's last comment, there is still a potential problem even though my patch significantly reduces the likelihood of it occurring.

Assert removed in a4db7025a9762c568c7bc9fdd3c64f4a60e31cfc.

Added the assert. The assert seems in order here - no hits in the lit tests or Vulkan CTS. There would have been hits in 194 lit tests if the assert had been placed here without the isInvariant check, which somewhat proves usefulness of the patch.

Added mir test.

Re-purposing patch to not add invariant loads to the map.