This is an archive of the discontinued LLVM Phabricator instance.

NVPTX: Refactor NVPTXInferAddressSpaces to check TTI
ClosedPublic

Authored by arsenm on Jan 25 2017, 8:31 PM.

Details

Reviewers
jlebar
Summary

Add a new TTI hook for getting the generic address space value.

I considered a more refined TTI hook that could check specific address space combinations, but I don't really have a use for it to justify the increase in complexity.

Diff Detail

Event Timeline

arsenm created this revision.Jan 25 2017, 8:31 PM
arsenm updated this revision to Diff 85861.Jan 25 2017, 8:47 PM

Add missing const

jlebar added inline comments.Jan 26 2017, 5:22 PM
include/llvm/Analysis/TargetTransformInfo.h
230

Nit, can we rephrase so the first sentence (a) says what the function does and (b) isn't a fragment?

230

s/ID value/ID/?

233

Should we say "legal but undesirable"? At which point should we say what we really mean, which is "slower than"?

236

Maybe I'm biased because I'm coming from the CUDA world, but "undesirable" doesn't really seem to capture what's going on as well as 'generic'., and indeed the comment uses "generic" to good effect.

What are you trying to get across with the name, exactly?

236

Maybe this should return optional<unsigned>?

lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp
444

If we used optional<unsigned> we wouldn't need this hack, either.

arsenm added inline comments.Jan 26 2017, 5:31 PM
include/llvm/Analysis/TargetTransformInfo.h
236

I was trying to avoid a second meaning of the term generic address space. LLVM already unfortunately clobbers the meaning of "generic" address space to mean addrspace(0) and all the assumptions it makes about it. This is generic/flat in a GPU world sense

arsenm updated this revision to Diff 86012.Jan 26 2017, 9:02 PM
jlebar edited edge metadata.Jan 26 2017, 10:51 PM

I like the new name and comment a lot. Do you think we should rename "GenericAddrSpace"?

I'm still not sold on returning -1 rather than using an optional<unsigned>, though. The meaning of

GenericAddrSpace = TTI.getFlatAddressSpace();
if (GenericAddrSpace == ADDRESS_SPACE_UNINITIALIZED)

is highly nonobvious, relying on the fact that ADDRESS_SPACE_UNINITIALIZED is the same as the default return value of getFlatAddressSpace(). Whereas if we used optional, it would become:

optional<unsigned> MaybeFlatAS = TTI.getFlatAddressSpace();
if (!MaybeFlatAS) return;
FlatAS = *MaybeFlatAS;

which I think pretty directly conveys what we're getting at?

arsenm updated this revision to Diff 86159.Jan 27 2017, 7:25 PM

Renaming

I like the new name and comment a lot. Do you think we should rename "GenericAddrSpace"?

I'm still not sold on returning -1 rather than using an optional<unsigned>, though. The meaning of

GenericAddrSpace = TTI.getFlatAddressSpace();
if (GenericAddrSpace == ADDRESS_SPACE_UNINITIALIZED)

is highly nonobvious, relying on the fact that ADDRESS_SPACE_UNINITIALIZED is the same as the default return value of getFlatAddressSpace(). Whereas if we used optional, it would become:

optional<unsigned> MaybeFlatAS = TTI.getFlatAddressSpace();
if (!MaybeFlatAS) return;
FlatAS = *MaybeFlatAS;

which I think pretty directly conveys what we're getting at?

I fixed the name of ADDRESS_SPACE_UNINITIALIZED to be UnknownAddressSpace in the cleanup commit which I think is more clear. I'd rather not deviate from the API/convention the other address space TTI functions already use (e.g. LSR defines the same constant and passes it to isLegalAddressingMode sometimes)

include/llvm/Analysis/TargetTransformInfo.h
236

Seems like overkill, and other places already use the -1 convention. Address spaces are only allowed to be 24-bits, so we don't need to spare every bit.

I fixed the name of ADDRESS_SPACE_UNINITIALIZED to be UnknownAddressSpace in the cleanup commit which I think is more clear. I'd rather not deviate from the API/convention the other address space TTI functions already use (e.g. LSR defines the same constant and passes it to isLegalAddressingMode sometimes)

The documentation as written doesn't ever say that we return -1 if the target doesn't have a flat AS. In fact it's not even explicit whether all targets have a valid "flat" AS -- maybe we just return 0 for "normal" targets.

If you don't want to return an optional, can you please fix the comments? In addition, I think we should use an explicit "-1" literal when we check the return value of getFlatAddressSpace. The UNINITIALIZED constant used in the pass does not mean the same thing as the -1 returned by getFlatAS, and conflating those two harms understanding.

I also think we should change the name back from "unknown AS" to "uninitialized AS". In the pass, we have a lattice of states, where "uninitialized" is at the top, the specific address spaces are in the middle, and "I don't know what the heck this thing is" is at the bottom. Things start in the "uninitialized" state, and they get to the "I don't know" state when we realize that they might be more than one thing, or when we have no idea where they came from.

"Unknown" fits the meaning of this "I don't know" state (it's a "known unknown"), so it's ambiguous whether "unknown AS" is the top or bottom of the lattice. "Uninitialized" is unambiguously the top, so I think it's a better name.

I fixed the name of ADDRESS_SPACE_UNINITIALIZED to be UnknownAddressSpace in the cleanup commit which I think is more clear. I'd rather not deviate from the API/convention the other address space TTI functions already use (e.g. LSR defines the same constant and passes it to isLegalAddressingMode sometimes)

The documentation as written doesn't ever say that we return -1 if the target doesn't have a flat AS. In fact it's not even explicit whether all targets have a valid "flat" AS -- maybe we just return 0 for "normal" targets.

Then the pass will run and do something nonsensical. I think this would break people using addrspacecast for GC etc.

arsenm updated this revision to Diff 86338.Jan 30 2017, 1:32 PM

Update documentation

jlebar accepted this revision.Jan 30 2017, 2:27 PM

lgtm, thanks for being patient with me.

This revision is now accepted and ready to land.Jan 30 2017, 2:27 PM
arsenm closed this revision.Jan 30 2017, 3:13 PM

r293563 + some function renames to match the new flat terminology