Adding Pierre as he worked on this recently as well.

Address @arsenm's comment

Address review feedback

Address review comments.

In D149986#4334274, @Pierre-vh wrote:

I think that if this is a new property of the GFX940/941 targets, and turning it off shouldn't be possible, we shouldn't even bother with a feature and just set a bool in the ST for those targets

In D149986#4329302, @arsenm wrote:

Should this be a feature set by default in the subtarget constructor instead? Should you be able to turn this off?

Issue was fixed by another change.

In D108322#3932212, @arsenm wrote:

Is this still relevant? Would this be better served by making the lower bound of amdgpu-waves-per-eu artificially increase the register usage?

looks good

Overall looks good.

lgtm, thanks!

In D145671#4181568, @yaxunl wrote:

Did you go through the deprecation process for this option? We need to send a documentation team first so that it is announced to the users. Then in the next release we can deprecate it.

LGTM, but please add a todo or a fixme (see comment below), thanks

LGTM, thanks

lgtm

lgtm, thanks

Also needs a test.

LGTM, thanks!

Please upload the full diff

Adding Jay

Need to also update: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td#L3626

LGTM, thanks

Do we want to tie this bit to a particular code object version? Code object v4 and up? Why did you pick code object v3 and up?

@arsenm, any additional feedback?

Clear kill flags for Src0 if its a reg.

Handle non register case and add a test for it.

In D127768#3582710, @rampitec wrote:

In D127768#3582672, @kzhuravl wrote:

In D127768#3582616, @rampitec wrote:

Why not to remove original v_cmp instead? It should have no other uses.

Original v_cmp_eq_u32 got removed in favor of v_cmpx_eq_u32. v_cmp_ne_u32 is left as is.

OK, I see. That's another instruction in between kills it..

What if src1 is not register?

In D127768#3582616, @rampitec wrote:

Why not to remove original v_cmp instead? It should have no other uses.

LGTM, can you wait and see if @rampitec has any additional comments?

In D127241#3570201, @foad wrote:

Can you add documentation to AMDGPUUsage in this patch as well?

That's in D127402 which is in review right now.

In D127241#3570194, @kzhuravl wrote:

In D127241#3564469, @Joe_Nash wrote:

In D127241#3564460, @rampitec wrote:

This looks like it affects gfx10 as well, while probably should not?

@kzhuravl Do you know about this?

GFX10 is only using COMPUTE_PGM_RSRC3_GFX10_PLUS.SHARED_VGPR_COUNT, the rest of the bits for GFX10 are reserved and must be 0.

GFX11 is using COMPUTE_PGM_RSRC3_GFX10_PLUS.INST_PREF_SIZE and others.

This is similar to how we do other compute program resource registers. E.g.:

...
  COMPUTE_PGM_RSRC1(BULKY, 24, 1),
  COMPUTE_PGM_RSRC1(CDBG_USER, 25, 1),
  COMPUTE_PGM_RSRC1(FP16_OVFL, 26, 1),    // GFX9+
  COMPUTE_PGM_RSRC1(RESERVED0, 27, 2),
  COMPUTE_PGM_RSRC1(WGP_MODE, 29, 1),     // GFX10+
  COMPUTE_PGM_RSRC1(MEM_ORDERED, 30, 1),  // GFX10+
  COMPUTE_PGM_RSRC1(FWD_PROGRESS, 31, 1), // GFX10+
...

The reason we have 2 different compute program resource register 3 is because GFX90A and GFX10 have completely different layout for those. So I think this header file should be fine.

Can you add documentation to AMDGPUUsage in this patch as well?

Thanks

In D127241#3564469, @Joe_Nash wrote:

In D127241#3564460, @rampitec wrote:

This looks like it affects gfx10 as well, while probably should not?

@kzhuravl Do you know about this?

LGTM, thanks

Need to update processor table: https://llvm.org/docs/AMDGPUUsage.html#processors
Need to update EF_AMDGPU_MACH_AMDGCN table: https://llvm.org/docs/AMDGPUUsage.html#amdgpu-ef-amdgpu-mach-table

Looks like this change is missing doc updates?

LGTM, thanks!

LGTM with clang formatting fixed

LGTM, unless @foad has any additional questions

LGTM with a minor nit

Do we need to add the test for it?

LGTM

LGTM

LGTM, thanks

lgtm

LGTM with the nit fixed

Adding Tony

LGTM, thanks for taking this over!

LGTM, thanks

Other than a TODO comment, LGTM, thanks

lgtm

ping

lgtm

LGTM, maybe give a day or so before merging this, so other folks have a chance to take a look

lgtm, thanks