Page MenuHomePhabricator

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

Authored by hakzsam on May 23 2018, 7:54 AM.

Details

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

Diff Detail

Event Timeline

hakzsam created this revision.May 23 2018, 7:54 AM
mareko accepted this revision.May 23 2018, 7:59 AM

LGTM.

This revision is now accepted and ready to land.May 23 2018, 7:59 AM
arsenm requested changes to this revision.May 23 2018, 9:36 AM

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

This revision now requires changes to proceed.May 23 2018, 9:36 AM

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?

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

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

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

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?

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

What's the status of this patch?

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

hakzsam updated this revision to Diff 161199.Aug 17 2018, 3:25 AM

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

Not sure if it's enough for Matt though.

arsenm requested changes to this revision.Aug 17 2018, 7:31 AM

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?

This revision now requires changes to proceed.Aug 17 2018, 7:31 AM
hakzsam updated this revision to Diff 161456.Aug 20 2018, 3:00 AM

v3: use static_assert()

arsenm accepted this revision.Aug 20 2018, 6:02 AM

LGTM

This revision is now accepted and ready to land.Aug 20 2018, 6:02 AM
jvesely requested changes to this revision.EditedAug 20 2018, 6:50 AM
jvesely added a subscriber: 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

This revision now requires changes to proceed.Aug 20 2018, 6:50 AM
hakzsam added a comment.EditedAug 20 2018, 6:56 AM

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 updated this revision to Diff 161545.EditedAug 20 2018, 12:54 PM

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 added inline comments.Aug 20 2018, 2:08 PM
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

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 added inline comments.Aug 20 2018, 2:28 PM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
53

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

hakzsam added inline comments.Aug 21 2018, 12:56 AM
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 added inline comments.Aug 21 2018, 7:21 AM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
53

The alias rules are identical to constant

jvesely added inline comments.Aug 21 2018, 7:39 AM
lib/Target/AMDGPU/AMDGPU.h
232

yes, MAX_AMDGPU_ADDRESS sounds OK.

hakzsam updated this revision to Diff 161745.Aug 21 2018, 9:26 AM

v5: rename MAX_COMMON_ADDRESS to MAX_AMDGPU_ADDRESS

jvesely accepted this revision.Aug 21 2018, 9:46 AM

v5: rename MAX_COMMON_ADDRESS to MAX_AMDGPU_ADDRESS

thanks

This revision is now accepted and ready to land.Aug 21 2018, 9:46 AM
mareko added inline comments.Aug 21 2018, 5:41 PM
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 added inline comments.Aug 22 2018, 12:03 AM
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

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

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

LGTM

hakzsam added a comment.EditedAug 22 2018, 9:10 AM

r340417