This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add address space based alias analysis pass
ClosedPublic

Authored by rampitec on Mar 17 2017, 2:14 PM.

Details

Summary

This is direct port of HSAILAliasAnalysis pass, just cleaned for style and renamed.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Mar 17 2017, 2:14 PM
vpykhtin added inline comments.Mar 17 2017, 2:23 PM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
64

the check can be done using xor

T1->isVectorTy() ^ T2->isVectorTy()

vpykhtin accepted this revision.Mar 17 2017, 2:23 PM

LGTM.

This revision is now accepted and ready to land.Mar 17 2017, 2:23 PM
arsenm added inline comments.Mar 17 2017, 2:26 PM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
2

Missing comment

14

doesn't match pass name

16–18

Should move this to the TargetMachine when adding it

45

flat and constant may alias, this isn't the OpenCL rules

59–60

Use cast<> if not checking it

72

formatting

79

Missin space

lib/Target/AMDGPU/AMDGPUAliasAnalysis.h
2–3

Missing comment

arsenm added inline comments.Mar 17 2017, 2:30 PM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
76–82

We should probably be marking all constant loads in the frontend with invariant metadata.

84–102

I think BasicAA will handle all of this, we should only need to handle the address spaces

test/CodeGen/AMDGPU/vectorize-global-local.ll
16–23

Should run instnamer on this

rampitec marked 8 inline comments as done.Mar 17 2017, 2:39 PM
rampitec added inline comments.
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
84–102

It was not handled really, that's why it got here...

rampitec updated this revision to Diff 92207.Mar 17 2017, 2:41 PM

Addressed comments.

arsenm added inline comments.Mar 17 2017, 2:46 PM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
84–102

I think this is only correct to do if F is a kernel

rampitec added inline comments.Mar 17 2017, 2:52 PM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
84–102

You probably right, that is safer. I will change it. BTW, I have checked this method in BasicAA, it does not check argument attributes.

rampitec updated this revision to Diff 92210.Mar 17 2017, 2:56 PM
rampitec marked 2 inline comments as done.
rampitec added a reviewer: arsenm.

Added check if function is a kernel before assuming constant argument memory.

rampitec added inline comments.Mar 17 2017, 3:04 PM
lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
45

I think in the future as an extension module metadata can be checked to see if source was OpenCL and return NoAlias for flat and constant.

arsenm accepted this revision.Mar 17 2017, 3:29 PM

LGTM

lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
45

I think we really just need to mark constant accesses with invariant. Anything checking the raw language is broken

77

This is redundant with the assert getPointerAddressSpace will do

rampitec marked an inline comment as done.Mar 17 2017, 3:51 PM
This revision was automatically updated to reflect the committed changes.