Yep, in SSA use of A means it dominates B so it seems correct to expect only narrowing of the exec mask. Let's try and see what happens.

Fixed, updated description of the patch.

Sorry for breaking the test, it turned out it has differences between two runs in what functions are left after the inliner, for example: barney exists on the first path but don’t exist on the second path. This cannot be handled with a single CHECK prefix.

replaced ARGPROMOTION prefix with CHECK, renamed %tmp IR values to %temp to avoid update warnings like "WARNING: Change IR value name 'tmp1' or use --prefix-filecheck-ir-name to prevent possible conflict with scripted FileCheck name"

From a cursory look at the implementation, does this handle unwinding properly?

store i32 0, ptr %arg
call void @may_unwind() readnone
store i32 1, ptr %arg

I believe promotion is not possible in this case, because there's no good way to provide the new value on the unwind path to the caller.

@arsenm I've added inoutargs2.ll test which is modified rewrite_out_arguments.ll. Original checks are saved with REF prefix, differences marked with "; ***" prefix, please take a look. Most of the differences are related to bitcasts which my patch doesn't support yet, I'm not sure if it should be included in this commit

LGTM.

Using IsArgGepInBound map just to hold a boolean seems expensive to me, can you just add a field to OriginalLoads value?

Updated opaque-ptr.ll test to use ALL prefix to simplify checks

@nikic please take a look now, I've addressed the issue with opaque pointers.

LGTM.

LGTM.

Can you check if the testcases in llvm/test/CodeGen/AMDGPU/rewrite-out-arguments.ll are redundant and/or covered by the ones you add here?

@arsenm I looked into the test and its not directly suitable for this pass as behaives differently, however the cases can be used for testing.

Some of the per review issues addressed.

LGTM

LGTM.

Looks almost good

In D117562#3272690, @vangthao wrote:
Right, by the time we hit pre-ra scheduler, occupany may already be decreased by MachineLICM and since the scheduler does not currently have any way to decrease register pressure of live-throughs, it would not have any ability to increase occupancy if it was decreased by MachineLICM hoisting from loops. Enforcing an occupancy target for RA will help in this case. Another issue is I believe the scheduler will allow for more VGPR usage as long as it does not decrease occupancy. This may hide some achievable occupancy if we do not handle this before scheduler or make scheduler aware that it can increase occupancy by sinking. I am not sure if disabling MachineLICM altogether in high RP situations would be the best solution here.

In D117562#3272067, @foad wrote:

Yes it is hard, but what alternative do we have? I think having the compiler make a decision about what occupancy it is trying to achieve, is better than not making any decision at all. And the scheduler already makes that decision, it is just not enforced.

In D117562#3271947, @foad wrote:

more comments to follow

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.