This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] add an NVPTX-specific alias analysis
Needs ReviewPublic

Authored by jlebar on Aug 27 2015, 3:29 PM.

Details

Summary

[NVPTX] Add NVPTX-specific alias analysis.

This analysis reports that two pointers don't alias if they are in
different non-generic address spaces.

This is motivated by the discussion in
http://lists.llvm.org/pipermail/llvm-dev/2015-August/089023.html.

Initial patch by Jingyue Wu, who based his change on a patch by Xuetian
Weng.

Diff Detail

Event Timeline

jingyue updated this revision to Diff 33364.Aug 27 2015, 3:29 PM
jingyue retitled this revision from to [NVPTX] add an NVPTX-specific alias analysis.
jingyue updated this object.
jingyue added subscribers: llvm-commits, eliben, meheff, broune.
jingyue updated this object.Aug 29 2015, 11:14 AM
eliben accepted this revision.Aug 31 2015, 3:39 PM
eliben added a reviewer: eliben.

lgtm

lib/Target/NVPTX/NVPTXAliasAnalysis.cpp
11 ↗(On Diff #33364)

s/no/do not/ ?

This revision is now accepted and ready to land.Aug 31 2015, 3:39 PM
jingyue updated this revision to Diff 34079.Sep 4 2015, 2:30 PM
jingyue edited edge metadata.

fixed some comments

hfinkel added inline comments.Sep 6 2015, 7:14 AM
lib/Target/NVPTX/NVPTXAliasAnalysis.cpp
118 ↗(On Diff #34079)

I'd highly prefer we make this more future-proof and actually check here only for the address spaces the target currently understands. If you want to enable this earlier in the pipeline, there may be address spaces that are used by upper layers (garbage collection schemes, distributed memory layers, etc.), which will go away prior to hitting CodeGen, but which may not share this strict NoAlias property with those understood by the target.

jingyue updated this revision to Diff 34125.Sep 6 2015, 11:19 AM

checks for address spaces our target understands

hfinkel added inline comments.Sep 6 2015, 9:47 PM
lib/Target/NVPTX/NVPTXAliasAnalysis.cpp
71 ↗(On Diff #34125)

Can you please explain why this DvrInterface == CUDA check is necessary? isKernelFunction always checks for NVVM metadata, and lacking that, a specific calling convention. Is that not enough?

jingyue added inline comments.Sep 6 2015, 10:14 PM
lib/Target/NVPTX/NVPTXAliasAnalysis.cpp
71 ↗(On Diff #34125)

Sure. As mentioned in the comment, if Obj is a CUDA kernel parameter, it must reside in global memory. But if the driver interface is NVCL, for example, a kernel parameter is not guaranteed to point to global memory.

This is also why we have a similar check in NVPTXLowerKernelArgs: https://github.com/llvm-mirror/llvm/blob/master/lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp#L201.

hfinkel added inline comments.Sep 6 2015, 10:24 PM
lib/Target/NVPTX/NVPTXAliasAnalysis.cpp
71 ↗(On Diff #34125)

Okay; I did not realize this might not hold for NVCL.

This it the only place you use TM in this pass, and thus, the only check that can't be done early in the pipeline. However, the driver interface variable is (at least currently) completely determined by the target triple. Is it worth checking the triple directly and dropping the direct TM dependency?

jingyue added inline comments.Sep 6 2015, 10:46 PM
lib/Target/NVPTX/NVPTXAliasAnalysis.cpp
71 ↗(On Diff #34125)

I wanted to avoid using TM too, but wonder how I can get it without passing in TargetMachine.

Never mind. I found Module has a getTargetTriple method.

jingyue updated this revision to Diff 34297.Sep 8 2015, 9:37 PM

avoid using NVPTXTargetMachine

jingyue added inline comments.Sep 8 2015, 9:39 PM
test/CodeGen/NVPTX/aa.ll
22 ↗(On Diff #34125)

%g2 is unnecessary any more, because with the target triple we can prove %g points to global.

hfinkel accepted this revision.Sep 15 2015, 7:17 AM
hfinkel edited edge metadata.

LGTM now too.

jingyue abandoned this revision.Mar 15 2016, 10:42 AM

It doesn't work with the new alias analysis infrastructure.

I didn't manage to find a way to run it with opt. The new AliasAnalysis needs to know and populate all potential alias analyses in a target-independent phase. However, NVPTXAliasAnalysis is target-dependent and not even visible to AliasAnalysis (e.g. compiling LLVM with NVPTX disabled).

It doesn't work with the new alias analysis infrastructure.

I didn't manage to find a way to run it with opt. The new AliasAnalysis needs to know and populate all potential alias analyses in a target-independent phase. However, NVPTXAliasAnalysis is target-dependent and not even visible to AliasAnalysis (e.g. compiling LLVM with NVPTX disabled).

Do you have an alternative plan?

It seems like we could add a feature in the new AA to query a target-provided analysis.

dberlin edited edge metadata.Mar 15 2016, 11:03 AM
dberlin added a subscriber: dberlin.

+1.
If we need this, we should make it work ;)

I'm not sure anyone sat down and said "it would be really bad to let
targets have their own AA", and make such a decision
(if they did, they didn't discuss it on llvm-dev)

jlebar commandeered this revision.Sep 10 2016, 12:06 PM
jlebar added a reviewer: jingyue.
jlebar added a subscriber: jlebar.

I have a framework that makes this work. Patches in a moment.

jlebar reclaimed this revision.Sep 10 2016, 12:13 PM
This revision is now accepted and ready to land.Sep 10 2016, 12:13 PM
jlebar updated this revision to Diff 70953.Sep 10 2016, 12:14 PM
jlebar edited edge metadata.

Update to work with the framework in D24441.

This new patch is basically the same as Jingyue's old patch, except for the addition of some tests and some boilerplate changes to make it work with the framework in D24441.

This revision now requires review to proceed.Sep 10 2016, 12:16 PM
jlebar updated this object.Sep 10 2016, 12:17 PM