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.

In D101115#2711972, @piotr wrote:

In D101115#2711964, @arsenm wrote:

In D101115#2711462, @piotr wrote:

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

I'm not worried about legacy, but whether it's redundant since it should have already run at this point

Some sinking is done as part of other passes, but I do not think this pass is set up to be run at any other point in our pass list.

Rebased, added comment in test.

In D101115#2711964, @arsenm wrote:

In D101115#2711462, @piotr wrote:

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

I'm not worried about legacy, but whether it's redundant since it should have already run at this point

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

Added test.

I suspect the compr version was used in the original shader, which was then shortened to the lit file.

Looks good, thanks.

LGTM

Fixed newline and added static/const.

Alternatively, you could create the set of blocks containing a kill in advance - by looking at function declarations in the module, and if a kill is found then checking its users to find blocks. Whether that would be better depends on how often hasKill() is called, though.

Rebased, changed strictwqm to strict_wqm and updated tests.

Added missing dot in the intrinsic name.

Rebased and updated tests.

Looks good to me (with two nits inline).

Commoned up the SI and GFX10-WAVE64 checks.

Updated commit message with more details and pasted the original error message.

Tests rewritten.

Add the underscore between strict and wwm. Restored the old tests for amdgcn_wwm.

This is the intended patch, no need to change AMDGPUISelDAGToDAG.cpp in the most recent version. Here, due to the updated check in isBoolSGPR the problematic combines that would strip zext will not happen (see SITargetLowering::performAddCombine/performSubCombine). This is similar to the very first approach I put up for a review.

Following the suggesstions of updating isBoolSGPR.

Is it worth adding a test to ensure old intrinsic still works?

See also D96258.

LGTM with a few nits - feel free to ignore them.

Thanks!

Sorry for the lack of activity lately - I plan to get back to this patch in the near future.

LGTM with a few more nits (inline).

I asked myself the same question - this was tried before - see https://reviews.llvm.org/D24544. I do not know any more details.

Added two more instructions: trunc and freeze (thanks @foad).