[PATCH] D138563: [IntrisicEmitter] Allow intrinsic types to be dependent on the data layout

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I have mixed feelings about this patch. It feels useful to support the typed matchers, mostly in the context of fixing iPTR (really we should have a more generally parameterized iPTR if we can't fully get rid of it)

> The alternative would be to overload all the remaining intrinsics but that
> causes a lot of test churn (especially for llvm.va_*/llvm.stack{save,restore}*)
> and should not be necessary since those intrinsics don't really need to
> be overloaded, the types just happen to depend on the data layout string.

I think this is the correct solution; I think llvm_ptr_ty should be removed entirely and all intrinsics should be mangled on the pointer address space. It's not composable and we've just been migrating an intrinsic at a time as needed. We've already migrated some intrinsics for the stack use case (e.g. int_frameaddress and lifetime markers) and this switches strategies for a different set so it introduces inconsistency. This is a roadblock to fully supporting per-alloca address spaces, which has come up before as potentially useful. The datalayout address spaces are really for situations where some generic code needs to create a new value without any context, which is not the case with an intrinsic call site.

This is also going to make these intrinsics syntactically behave differently based on the datalayout. We tried something similar with program address spaces, such that the assembler currently interprets an undecorated function's address space differently based on the datalayout and I'm currently hating it. It's inconsistent and confusing and I'll likely post a patch to undo it, such that functions are always printed with an explicit addrspace(N) if it's non-0.

Assuming we do want to go with this approach, this is missing various assembler and verifier tests for all the impacted intrinsics.



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:189
+  // Must be one of 'A', 'G', 'P' (the DataLayout-defined address spaces)
+  string AddrSpace = addrspace;
+}
----------------
Just AddrSpace sounds like it's expecting the raw number, like in LLVMQualPointerType


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:272-274
+def llvm_prog_ptr_ty   : LLVMDataLayoutPointerType<llvm_i8_ty, "P">; // (prog)i8*
+def llvm_alloca_ptr_ty : LLVMDataLayoutPointerType<llvm_i8_ty, "A">; // (alloca)i8*
+def llvm_global_ptr_ty : LLVMDataLayoutPointerType<llvm_i8_ty, "G">; // (globals)i8*
----------------
This part feels useful


================
Comment at: llvm/utils/TableGen/IntrinsicEmitter.cpp:387
-      AddrSpace = R->getValueAsInt("AddrSpace");
-      assert(AddrSpace < 256 && "Address space exceeds 255");
-    }
----------------
This is a separate diagnostic improvement that should be split out


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138563