User Details
- User Since
- Dec 4 2018, 6:02 AM (185 w, 5 d)
Thu, Jun 16
Tue, May 31
May 20 2022
Just verified this patch fixes the rendering corruption caused by D114643 - thanks!
May 17 2022
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.
May 13 2022
Looks good to me - the extra s_cselect's generated are worth the complexity arising from this patch.
Apr 29 2022
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.
Apr 28 2022
Apr 27 2022
LGTM
Apr 26 2022
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.
Apr 25 2022
Mar 22 2022
Do you have any data on the compile time impact?
Mar 1 2022
Feb 18 2022
LGTM
LGTM
LGTM
Feb 17 2022
Feb 16 2022
LGTM
LGTM
LGTM
Feb 7 2022
So that indicates an improvement in the average vgpr count.
Feb 4 2022
LGTM
Jan 28 2022
LGTM
Jan 27 2022
LGTM
Jan 19 2022
Jan 18 2022
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%.
Nov 3 2021
Nov 2 2021
Ping.
Oct 26 2021
LGTM.
Oct 22 2021
Tests pre-committed in 7457fe3dd44a0bc4b0296149c48188accefda5fa.
Oct 18 2021
Oct 15 2021
Added -global-isel-abort=0 and restored the tests.
Oct 14 2021
Sounds good to me. The runtime improvement from clustering is notoriously difficult to assess, but your static data shows some potential benefit.
Sep 30 2021
Sep 23 2021
Sep 21 2021
Sep 20 2021
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.
Sep 17 2021
The patch looks good to me (modulo the lint warnings).
Sep 14 2021
The change makes sense to me in general.
Sep 7 2021
Sep 2 2021
Aug 30 2021
Add test.
Jun 29 2021
Test case added.
Jun 14 2021
May 12 2021
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.
May 11 2021
May 10 2021
Added mir test.
May 7 2021
Re-purposing patch to not add invariant loads to the map.