Following the discussion at https://reviews.llvm.org/D97342.

LGTM

Looks good either but as with previous patch I think setting HasClusteredNodes to true to avoid discovering it's value is a bit misleading.

Looks good, but should we use just a single dedicated pass over SUs to check if there're clustered ops after first scheduling to make the logic slightly easier?

Jay, Matt do you have objections with this patch?

Fixed formatting issue

Fixed formattind and lint issues.

Added test, more review issues fixed.

In D89386#2332694, @rampitec wrote:

In D89386#2331649, @vpykhtin wrote:

In D89386#2330465, @rampitec wrote:

Actually do we have a test with a call in between def and use?

Probably not, how a call looks like at this stage?

It should be s_setpc_b64 at this point and it is a terminator and branch. Although I am not sure it can be detected as an EXEC change.

In D89386#2330465, @rampitec wrote:

Actually do we have a test with a call in between def and use?

Per review fixes.

I've found the case when execMayBeModifiedBeforeAnyUse randomly leads to a coredump, which is hard to debug. Most likely it's because an instruction beyond the end of a basic block is accessed. This means that the first loop calculates some instructions twice and I was wrong assuming use_nodbg_instructions doesn't repeat them. In fact there is no code in MachineRegisterInfo::verifyUseList that ensures that uses belonging to one instruction should be sequent in the use list nor the traces of such ordering can be found in MachineRegisterInfo::addRegOperandToUseList. I'm going to fix this code.

If that piece of code turns out to be mandatory, we'll find out soon enough and we would get a test case :).

Sorry, I had to explain the context around the modified code.

HI Quentin,

ping

In D82258#2230981, @qcolombet wrote:

Honestly, I have no idea one way or another. I'm leaning toward @arsenm's theory that the implicit def may be needed in case the full register is never fully covered.
But I would need to dig deeper into this.

Rebased, updated per review comments.

In D82258#2224137, @arsenm wrote:

Can you remove the commit message from the previous commit from this message? I found it confusing to read it here

ping

Ok, let's keep it simple, no much benefit with maps. LGTM.

Overally looks good

I have no objections but I'll ask Dmirty to check this from asm/dasm side.

In D82258#2148331, @arsenm wrote:

It would help if 9d7bc0874cf20f44cd331c77f5a003b4c4b262bd had a test...

I'm somewhat suspicious, since I have seen cases where the implicit def is needed in cases where the full register is never fully covered. However, it's possible this is still a leftover from before DetectDeadLanes was added

ping

Given than the condition here is rare, I'm ok with the patch, reworking undef handling in scheduler is a massive work.

I understood your patch. Generally I think patching LIS with cleared undef flags ins't right at first place because the semantic of register lifetime is ruined. After the first move there should be an undef flag set at the %1.sub2 = IMPLICIT_DEF instruction which would break lives of all subregs live at this point. Can we drop updating LIS during scheduling and recreate it from scratch? Or may be fully recreate only the intervals for the registers involved in moves when the undef flag is patched after scheduling?

Can you please add a LiveInterval dump (possibly truncated) for the test before and after each move to this review? It's a bit hard to follow what happens there.

Update before commit: used isIdenticalTo, rebased.

Ping

Ping

Rebased, added check if use is Src0 or Src1

Indeed, thanks Jay, updated.

Rebased, per review issues fixed, moved VNInfo::print definition ot of class.

split to parts

In D82451#2113831, @foad wrote:

1. skip multiple per instruction DPP register usage.
2. don't combine when DPP register is used as part of superreg/supersubreg.

I had to refactor the code to fix the first issue,

Would it make sense to split this patch into two or three? Refactoring, fix bug 1, fix bug 2?

Rebased, per review issues fixed:

Updated patch. Everything is done except I decided to left readsM0 check for the no-ret atomics case.

The SI_INIT_M0 is still used for instructions that I mention in hasNonDefaultM0. Comments say it was introduced to produce S_MOV_B32 m0 so the CSE could join them.

LGTM

LGTM

LGTM.

LGTM.

LGTM.

LGTM, assuming Matt's concern is addressed.

I cannot follow all the consequences of adding the landing pad, but this looks a robust solution since it's hard to find appropriate insertion point for s_or_saveexec instruction in the beginning of SI_ELSE constaining block.

LGTM.

LGTM, Thanks.

LGTM.

LGTM, thanks.

LGTM.

LGTM

LGTM.

Almost LGTM. Do you need those liveins reorderings?

Looks good, but the test would be nice to have.

LGTM.

updated the diff per comment. Used utils/update_llc_test_checks.py tool to update autogenerated test.

added test fix.