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

ping

ping

+Mark,Ron

This change introduces the regression in OpenCL conformance test: basic - kernel_memory_alignment_local. Does it require any corresponding runtime changes?

lgtm

Can you also update summary to drop AMDGPULLVMExtensionsForHeterogeneousDebugging?

Any reason you are bringing in heterogeneous debugging bits in this change?

Can you upload a diff with the context?

In D117797#3258546, @arsenm wrote:

Breaks how / on what values?

LGTM, I have looked through the implementation, and it looks like a correct revert of https://reviews.llvm.org/rG640beb38e7710b939b3cfb3f4c54accc694b1d30. I have looked through most tests, but not all. Can you make sure to run lit tests before pushing your change? Thank you

Add @tpr to look at pal metadata changes

LGTM with a nit below. Thanks!

The metadata treats the minimum as 16?

Agreed on needing a lit test

ping

LGTM, thanks!

Probably need docs https://llvm.org/docs/AMDGPUUsage.html#llvm-ir-attributes

ping

Address review feedback.

In D105507#2860701, @rampitec wrote:

Also needs disassembler change.

lgtm

Address review feedback.

In D102306#2760806, @tra wrote:

In D102306#2760775, @msearles wrote:

In D102306#2758166, @JonChesterfield wrote:

Is there documentation for the mapping from product names to gfx numbers?

See the Processors section of https://llvm.org/docs/AMDGPUUsage.html

It appears to be missing some GPUs. None of WX series are listed, for example.

LGTM, but please run clang-format, thanks!

In D101304#2729931, @nickdesaulniers wrote:

Hi, llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v3.s is failing for me.
/android0/llvm-project/llvm/build/bin/llvm-mc: error: : error: unable to get target for 'amdgcn-amd-amdhsa', see --version and --triple.
/android0/llvm-project/llvm/build/bin/llvm-readobj: error: '<stdin>': The file was not recognized as a valid object file
FileCheck error: '<stdin>' is empty.

In D101304#2733333, @Conanap wrote:

In D101304#2733314, @kzhuravl wrote:

In D101304#2733289, @Conanap wrote:

Hello,

The reland version of this patch for which I cannot find the differential of has broken LLVM::note-amd-valid-v2.test on PowerPC:

/home/buildbots/ppc64be-clang-test/clang-ppc64be/llvm/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test:17:14: error: LLVM-NEXT: expected string not found in input
# LLVM-NEXT: AMD HSA Code Object Version: [Major: 2, Minor: 1]
             ^
<stdin>:15:68: note: scanning from here
 Type: NT_AMD_HSA_CODE_OBJECT_VERSION (AMD HSA Code Object Version)
                                                                   ^
<stdin>:16:2: note: possible intended match here
 AMD HSA Code Object Version: [Major: 33554432, Minor: 16777216]
 ^
Input file: <stdin>
Check file: /home/buildbots/ppc64be-clang-test/clang-ppc64be/llvm/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test
-dump-input=help explains the following input dump.
Input was:
<<<<<<
           .
           .
           .
          10:  Offset: 0x40 
          11:  Size: 0x18 
          12:  Note { 
          13:  Owner: AMD 
          14:  Data size: 0x8 
          15:  Type: NT_AMD_HSA_CODE_OBJECT_VERSION (AMD HSA Code Object Version) 
next:17'0                                                                        X error: no match found
          16:  AMD HSA Code Object Version: [Major: 33554432, Minor: 16777216] 
next:17'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
next:17'1      ?                                                                possible intended match

Seems like a small fix but if you could have a push for it soon that'd be great. Thanks! FYI @rksharma

Hi,

Reland patch fixed an issue if AMDGPU backend was not built, which was a one liner (I reverted original patch to not create the noise).

Seems like it is an endianness issue.

Unfortunately I don't have PowerPC, will adding these lines in note-amd-valid-v2.test mark it as XFAIL for you:

diff --git a/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test b/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test
index 688bbd50994c..f9cac212ce9f 100644
--- a/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test
+++ b/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test
@@ -1,6 +1,9 @@
 ## This test is checking the handling of valid note entries for AMDGPU code
 ## object v2.

+## Big endian not supported.
+# XFAIL: host-byteorder-big-endian
+
 # RUN: yaml2obj %s -o %t.o
 # RUN: llvm-readobj --notes %t.o | FileCheck %s --match-full-lines --check-prefix=LLVM
 # RUN: llvm-readelf --notes %t.o | FileCheck %s --match-full-lines --check-prefix=GNU

?

That would fix it yes, thank you!
Another possible solution would be to add a BE specific runline, but I'm not really familiar with the AMD test files so it's just a suggestion.

In D101304#2733289, @Conanap wrote:

Hello,

The reland version of this patch for which I cannot find the differential of has broken LLVM::note-amd-valid-v2.test on PowerPC:

/home/buildbots/ppc64be-clang-test/clang-ppc64be/llvm/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test:17:14: error: LLVM-NEXT: expected string not found in input
# LLVM-NEXT: AMD HSA Code Object Version: [Major: 2, Minor: 1]
             ^
<stdin>:15:68: note: scanning from here
 Type: NT_AMD_HSA_CODE_OBJECT_VERSION (AMD HSA Code Object Version)
                                                                   ^
<stdin>:16:2: note: possible intended match here
 AMD HSA Code Object Version: [Major: 33554432, Minor: 16777216]
 ^
Input file: <stdin>
Check file: /home/buildbots/ppc64be-clang-test/clang-ppc64be/llvm/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test
-dump-input=help explains the following input dump.
Input was:
<<<<<<
           .
           .
           .
          10:  Offset: 0x40 
          11:  Size: 0x18 
          12:  Note { 
          13:  Owner: AMD 
          14:  Data size: 0x8 
          15:  Type: NT_AMD_HSA_CODE_OBJECT_VERSION (AMD HSA Code Object Version) 
next:17'0                                                                        X error: no match found
          16:  AMD HSA Code Object Version: [Major: 33554432, Minor: 16777216] 
next:17'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
next:17'1      ?                                                                possible intended match

Seems like a small fix but if you could have a push for it soon that'd be great. Thanks! FYI @rksharma