AMDGPU: bump AS.MAX_COMMON_ADDRESS to 6 since 32-bit addr space

[hakzsam]

Summary

32-bit constant address space is declared as 6, so the
maximum number of address spaces is 6, not 5.

Fixes "LLVM ERROR: Pointer address space out of range".

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106630

[mareko]

LGTM.

[arsenm]

Needs a test, preferably the full set of AA checks with 32 bit constant

[hakzsam]

In D47261#1109743, @arsenm wrote:

Needs a test, preferably the full set of AA checks with 32 bit constant

Sure., but can you give me more details about what you want? Is there already an example that I could start from?

[arsenm]

Needs a test, preferably the full set of AA checks with 32 bit constant

In D47261#1112296, @hakzsam wrote:

In D47261#1109743, @arsenm wrote:

Needs a test, preferably the full set of AA checks with 32 bit constant

Sure., but can you give me more details about what you want? Is there already an example that I could start from?

Unfortunately it looks like the commit that added this didn't actually add a proper test for this although I thought there was one that would be easy to add to. I would like there to be a test that purely tests the results of alias queries, like those found in test/Analysis/BasicAA or test/Analysis/ScopedNoAliasAA/basic.ll

[t-tye]

In D47261#1112309, @arsenm wrote:

Needs a test, preferably the full set of AA checks with 32 bit constant

In D47261#1112296, @hakzsam wrote:

In D47261#1109743, @arsenm wrote:

Needs a test, preferably the full set of AA checks with 32 bit constant

Sure., but can you give me more details about what you want? Is there already an example that I could start from?

Unfortunately it looks like the commit that added this didn't actually add a proper test for this although I thought there was one that would be easy to add to. I would like there to be a test that purely tests the results of alias queries, like those found in test/Analysis/BasicAA or test/Analysis/ScopedNoAliasAA/basic.ll

Given that @yaxunl has done the changes so there is now only one address space mapping, this code can be simplified (and corresponding declarations in AMDGPU.h) to remove support for dynamic address space mappings. So if etsts were to be added would likely be easier to do that for the simplified code?

[mareko]

FYI, this bug is also reproducible on Mesa OpenGL now.

[nhaehnle]

What's the status of this patch?

[mareko]

The patch is missing a test. shader-db can reproduce it.

[hakzsam]

v2: add a very simple test for 32-bit addr space

Not sure if it's enough for Matt though.

[arsenm]

Ideally the test should be moved into somewhere in test/Analysis but that can be a separate patch

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

72

This can be a static assert which would have caught this in the first place?

[hakzsam]

v3: use static_assert()

[arsenm]

LGTM

[jvesely]

NACK. This patch is clearly wrong.
MAX_COMMON_ADDRESS is used in AMDGPUAAResult::ASAliasRulesTy::getAliasResult to filter indices to the ASAliasRules table which is 6x6. Allowing address space 6 leads to out of bounds access to the array.

This has been reported and answered in https://bugs.llvm.org/show_bug.cgi?id=38113

[hakzsam]

In D47261#1205868, @jvesely wrote:

NACK. This patch is clearly wrong.
MAX_COMMON_ADDRESS is used in AMDGPUAAResult::ASAliasRulesTy::getAliasResult to filter indices to the ASAliasRules table which is 6x6. Allowing address space 6 leads to out of bounds access to the array.

This has been reported and answered in https://bugs.llvm.org/show_bug.cgi?id=38113

You are right, we can get out of bounds access with that change. I guess the correct fix is to update the rules table?

EDIT: This should be fixed with https://reviews.llvm.org/D50974

[hakzsam]

Both patches (r340171 and r340172) have been reverted with r340202 because it will be easier for a backport.
This new version squashes these two and includes the alias rules table fix (ie. out of bounds access).

v4:

• fix compilation issues (r340172)
• fix out of bounds access (D50974)

[jvesely]

lib/Target/AMDGPU/AMDGPU.h

232

Same comment as D50974. R600 does not use CONSTANT_ADDRESS_32BIT so this value should not be called MAX_COMMON_ADDRESS

[jvesely]

Please add a reference to llvm bug https://bugs.llvm.org/show_bug.cgi?id=38113
as well as correct "Differential Revision" tag when committing.

[mareko]

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

53

Can you set that "Constant 32-bit" does not alias any other address space?

[hakzsam]

lib/Target/AMDGPU/AMDGPU.h

232

Do you want to rename MAX_COMMON_ADDRESS to MAX_AMDGPU_ADDRESS or something?

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

53

Alias rules for "constant 32-bit" should be similar to "constant", no?

[arsenm]

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

53

The alias rules are identical to constant

[jvesely]

lib/Target/AMDGPU/AMDGPU.h

232

yes, MAX_AMDGPU_ADDRESS sounds OK.

[hakzsam]

v5: rename MAX_COMMON_ADDRESS to MAX_AMDGPU_ADDRESS

[jvesely]

In D47261#1207743, @hakzsam wrote:

v5: rename MAX_COMMON_ADDRESS to MAX_AMDGPU_ADDRESS

thanks

[mareko]

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

53

32-bit allocations use a separate allocator, so the address ranges of 32-bit and 64-bit allocations are different. They can't alias.

32-bit allocations could alias different 32-bit allocations. For example, "Global 32-bit" could alias "Constant 32-bit", but "Global" can't alias "Global 32-bit". addrspacecast for expanding a pointer isn't allowed as far as I know.

[arsenm]

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

53

I'm not sure we should be basing the backend aliasing decisions on this kind of property. I think this would require some careful thought, so I would much rather leave it as identical to constant for now.

Specifying 32-bit and 64-bit allocations don't alias should theoretically be possible with alias sets separate from the address space aliasing properties

[hakzsam]

Should I push the patch as is? Or does it need another revision?

[arsenm]

In D47261#1208784, @hakzsam wrote:

Should I push the patch as is? Or does it need another revision?

LGTM

[hakzsam]

r340417

lib/Target/AMDGPU/AMDGPU.h

 
   enum : unsigned {
     // The maximum value for flat, generic, local, private, constant and region.
-    MAX_COMMON_ADDRESS = 5,
+    MAX_COMMON_ADDRESS = 6,
 
     GLOBAL_ADDRESS = 1,   ///< Address space for global memory (RAT0, VTX0).
     CONSTANT_ADDRESS = 4, ///< Address space for constant memory (VTX2)

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

   /* Region   */ {MayAlias, NoAlias , NoAlias , NoAlias,  MayAlias, NoAlias},
   /* Private  */ {MayAlias, NoAlias , NoAlias , NoAlias , NoAlias , MayAlias}
   };
-  assert(AS.MAX_COMMON_ADDRESS <= 5);
+  assert(AS.MAX_COMMON_ADDRESS <= 6);
   if (AS.FLAT_ADDRESS == 0) {
     assert(AS.GLOBAL_ADDRESS   == 1 &&
            AS.REGION_ADDRESS   == 2 &&