[PATCH] D29164: NVPTX: Refactor NVPTXInferAddressSpaces to check TTI

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

> 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.


https://reviews.llvm.org/D29164