[PATCH] D91428: Add support for multiple program address spaces

Fri Nov 13 12:19:05 PST 2020

tlively added a comment.

I think this is a good direction overall, and I'm glad the code doesn't become any messier with this change. I do think it would be good to also email llvm-dev about this change to get general feedback and make sure it doesn't require a full RFC.

================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1470
+
   // For calls we also accept variables in the program address space.
   if (IsCall && isa<PointerType>(Ty)) {
----------------
Or maybe this comment can be removed since it is subsumed by the comment you added just below.

================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1491
                    getTypeString(Val->getType()) + "' but expected '" +
-                   getTypeString(SuggestedTy) + "'");
+                   getTypeString(Ty) + "'");
   return nullptr;
----------------
It would be good to add a test for this error message (if one doesn't already exist). It's not obvious to me that the expected type in the error message will always have a correct program address space.

================
Comment at: llvm/lib/IR/DataLayout.cpp:476
         return Err;
+      ProgramAddrSpaces.push_back(pa);
       break;
----------------
It would be good to check for repeats here. As it is, `hasNonZeroProgramAddressSpace()` would return `true` for "P0-P0" .

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91428/new/

https://reviews.llvm.org/D91428