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.
Paths
| Differential D29164
NVPTX: Refactor NVPTXInferAddressSpaces to check TTI ClosedPublic Authored by arsenm on Jan 25 2017, 8:31 PM.
Details
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
Comment Actions 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 added a child revision: D29166: NVPTX: Move InferAddressSpaces to generic code.Jan 26 2017, 11:00 PM Comment Actions
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)
Comment Actions
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. Comment Actions
Then the pass will run and do something nonsensical. I think this would break people using addrspacecast for GC etc. This revision is now accepted and ready to land.Jan 30 2017, 2:27 PM
Revision Contents
Diff 86338 include/llvm/Analysis/TargetTransformInfo.h
include/llvm/Analysis/TargetTransformInfoImpl.h
include/llvm/CodeGen/BasicTTIImpl.h
lib/Analysis/TargetTransformInfo.cpp
lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp
lib/Target/NVPTX/NVPTXTargetTransformInfo.h
test/CodeGen/NVPTX/access-non-generic.ll
|
Nit, can we rephrase so the first sentence (a) says what the function does and (b) isn't a fragment?