Ping.

In D80485#2342195, @amyk wrote:

In D80485#2341858, @piotr wrote:

Hi! This commit causes problems for AMDGPU backend - see attached file

repro.ll1 KBDownload

. Any ideas before I start investigating this in detail?

LLVM ERROR: Cannot select: t56: i16 = mulhs t42, Constant:i16<-32509>

t42: i16 = truncate t67
  t67: i32 = add t66, t28
    t66: i32 = add t37, t34
      t37: i32 = shl nuw nsw t12, Constant:i32<13>
        t12: i32,ch = CopyFromReg t0, Register:i32 %5
          t11: i32 = Register %5
        t36: i32 = Constant<13>
      t34: i32 = shl nuw nsw t10, Constant:i32<7>
        t10: i32,ch = CopyFromReg t0, Register:i32 %4
          t9: i32 = Register %4
        t26: i32 = Constant<7>
    t28: i32 = add t16, t27
      t16: i32,ch = CopyFromReg t0, Register:i32 %7
        t15: i32 = Register %7
      t27: i32 = shl t8, Constant:i32<7>
        t8: i32,ch = CopyFromReg t0, Register:i32 %3
          t7: i32 = Register %3
        t26: i32 = Constant<7>
t52: i16 = Constant<-32509>

Hi, I agree with the suggestion by @dmgreen. Could you see if that works?

Hi! This commit causes problems for AMDGPU backend - see attached file

Related to D78091.

Renamed variable to avoid a multiline assignment.

Good spot - I looked at the shader database and indeed the pattern with two connected setcc is frequent enough to special case it so I updated the patch.

Invert the condition.

Ah now I see what you meant, sorry.

Ping.

Simplified test and added 5 more.

In D87882#2282886, @rampitec wrote:

In D87882#2282818, @piotr wrote:

In D87882#2282278, @rampitec wrote:

Doesn't loop block self dominate?

You mean, why the condition "MDT.dominates(From, To)" returns false if From and To are in the same BB? Inside that function there is a bb dominance check if the instructions are in different blocks, but if the instructions are in the same block the code checks whether From comes before To.

The bug I am trying to fix is simplified in the test m0-in-loop-0 where before my patch SI_INIT from line 333 was incorrectly removed. Although "m0 = COPY" comes after SI_INIT in that bb, this is a loop and in the second iteration DS_WRITE would see a clobbered m0, so SI_INIT has to be preserved.

That sounds like a bug in the dominate() to me.

In D87882#2282278, @rampitec wrote:

Doesn't loop block self dominate?

I think you are right - I moved the fix to the better place.

Switched to SIInstrInfo::fixImplicitOperands, thanks.

As suggested, moved the code to buildShrunkInst where the instruction is created.

Yes, this patch is not safe in general, because in some cases isel produces patterns where all bits of an SCC source are significant.

@alex-t for awareness, as the code I am changing here was mentioned in https://reviews.llvm.org/D82194#2117180.

Renamed the option and variable.

Hmm, I am getting Vulkan CTS failures with this patch. I'll try to gather more information.

Looks good to me as a stop-gap solution.

Excellent test coverage, thank you for doing that.

Heads-up, this change slightly conflicts textually with my change to select s_cselect which I just recommitted following a bug-fix in 0045786f146e78afee49eee053dc29ebc842fee1.

Reworded comment, added a TODO.

Added MIR test to test undef SCC, added minor tweaks in lowerSelect and rebased.

Addressed review comments.

LGTM

Hi, This commit makes some compilations go into an infinite loop. I will try to prepare a reproducer.

The patch has caused vgpr count to go up significantly in real-world graphics content. Agreed with @cdevadas to revert for now.

Fix formatting.

See D71192#1895125 for more context.

The reported example comes from one of graphicsfuzz CTS tests, so I think it is fine to treat it as an unrealistic corner case where performance is not critical.

Hi @cwabbott,
This commit causes a GPU hang on amdvlk in one test, due to the missing "done" bit on the normal export. I think in the attached case the patch incorrectly classifies the export as being in an infinite loop.

Silenced clang-tidy warnings.

Addressed review comments.

I intend to merge the patch soon, based on the fact that the patch was reviewed in D70315. Compared to that version, 3 problematic cases were removed and tests updated.

Ping.