In D151797#4383969, @arsenm wrote:

Missing test updates?

Sorry for the delay. I believe D151797 should allow this to proceed.

LGTM

LGTM

I assume D129208 is the test case for this, so it does not need any new tests?

My intention is to sit down and try make WQM work with this.
It is a non-trivial change, so no promises, but I will try to look at it in the next few days.

LGTM

LGTM, but one question about the diff preparation.

• Exclude global objects without metadata from unmapped set
• Add assertion to cover the case of intrinsics with metadata

In D145318#4181503, @tejohnson wrote:

can you re-upload with context?

• Add missing context

• Add missing erasure of functions which are erased through renaming.

• Remove unnecessary erase calls, simplifying implementation
• Extend comment

In D145318#4169797, @tejohnson wrote:

Is this a case where we didn't in the past have metadata attachments on declarations requiring mapping, and now we do? Also, the example uses a "!types" metadata - I'm familiar with !type metadata on definitions, but haven't seen !types before - is this new metadata? I can't find any uses of it in the test suite or any documentation.

Unfortunately this interferes with WQM mode change insertion.
You can see this in the reordered s_or + s_and instruction pairs.
I guess this was always a risk with block splitting.

Thank you for implementing this.
I have left a few minor comments inline.

@arsenm - please can you elucidate the changes you'd like made.

• Extend available base class range by one

• Address reviewer comments

• Avoid passing virtual registers to getPhysRegBaseClass in IsCopyFromSGPR

• Use assertion instead of range check when resolving base classes

• Fix trivial off by one error in base class table

• Address reviewer feedback
• Add lit test

• Remove stray code from earlier revision

• Turn this into a proof of concept for D139616

In D139422#3974201, @arsenm wrote:

In D139422#3974200, @critson wrote:

Ideally this would be a static table, but the changes required to TableGen to understand register classes for this are non-trivial.

Why? The regclass structure is exactly represented there already

Ideally this would be a static table, but the changes required to TableGen to understand register classes for this are non-trivial.

LGTM

• Add test llvm/test/Transforms/Sink/invariant-load.ll

-> D135133

Logically I'd like some kind of control over the sinking process, but I guess that can be handled by removing invariant metadata as appropriate.

In D134151#3823142, @foad wrote:

In D134151#3823083, @critson wrote:

Beyond DPP which other exotic forms are you looking for?

The ones I can find at the moment are:

V_ADDC_U32_sdwa
V_CNDMASK_B16_sdwa
V_CNDMASK_B32_sdwa
V_SUBBREV_U32_sdwa
V_SUBB_U32_sdwa

• Change mitigation strategy to s_waitcnt_depctr after SALU

In D134151#3823059, @foad wrote:

Sure, although...

There is still a small problem that the list of instructions here is missing exotic forms like V_CNDMASK_B32_dpp.

... this still bothers me.

• Add DPP forms and associated tests

In D134780#3821260, @foad wrote:

Out of ~20000 pipelines, ~2000 had higher VGPR usage with threshold 2 and ~1000 had lower VGPR usage.

That's weird. I can't see why enabling NSA would consistently cause higher vgpr usage.

FYI I looked at one case where this happens and it was caused by GCNNSAReassign making strange (well, different) decisions. So now I need to try to understand what that pass does.

• Address reviewer feedback

In D134780#3820209, @foad wrote:

Seems reasonable.

I would also be in favour of changing the default to 2. That would tend to introduce fewer "mov"s to shuffle data around, at the expense of using a larger encoding on average for mimg instructions. But I think that is a good trade-off to make for the sake of performance.

So this was based on dealing with the temporary 128 VGPR limit on GFX11, which has now been removed.
On that basis I will probably abandon this revision; however, I do still have concerns about this magic number 4.

I'm not sure where this is currently tested to add a new test.
If any reviewers know?

• Address reviewer comments

In D134025#3795210, @arsenm wrote:

What context is passing this through?

I think the code is correct, but this needs a MIR test checking the behaviour of the pass.

Compilation testing on graphics shader corpus (~10k pipelines) for Navi10.