[PATCH] D105423: Add support for Opaque as a LowLevelType

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

In D105423#2862011 <https://reviews.llvm.org/D105423#2862011>, @arsenm wrote:

> In D105423#2862010 <https://reviews.llvm.org/D105423#2862010>, @pmatos wrote:
>
>> In D105423#2861795 <https://reviews.llvm.org/D105423#2861795>, @arsenm wrote:
>>
>>> I'm not sure what an opaque type means. Do you have an IR sample?
>>
>> Sure - for example, from how we represent externrefs (opaque object references in WebAssembly):
>>
>>   %extern = type opaque
>>   %externref = type %extern addrspace(1)* ;; addrspace 1 is nonintegral
>>   @externref_global = local_unnamed_addr addrspace(2) global %externref undef
>>   
>>   define %externref @return_externref_global() {
>>     ;; this generates a global.get of @externref_global
>>     %ref = load %externref, %externref addrspace(2)* @externref_global
>>     ret %externref %ref
>>   }
>
> But here it's just a pointer with an address space and you aren't actually using the opaque type as a value. Why can't you just treat this as a regular pointer?

IIUC, the "opaque" type (in the sense of this patch) here is %externref, which lowers to a zero-sized (i.e. no bit representation) MVT. So it is a regular pointer except that it has zero size, which requires special care here and elsewhere in https://reviews.llvm.org/D104797. From @pmatos's latest comments on that other revision, perhaps an alternative to this patch would be to allow scalar LLTs to have zero size?



================
Comment at: llvm/include/llvm/Support/LowLevelTypeImpl.h:293-294
   /// described in static const *Field variables. Each of these variables
-  /// is a 2-element array, with the first element describing the bitfield size
+  /// is a 3-element array, with the first element describing the bitfield size
   /// and the second element describing the bitfield offset.
+  typedef int BitFieldInfo[3];
----------------
Why did the number of elements change? What is the third element for? It would be good to update the comment.


================
Comment at: llvm/include/llvm/Support/LowLevelTypeImpl.h:297
   ///
   /// This is how the bitfields are packed per Kind:
   /// * Invalid:
----------------
How are the bitfields packed for `opaque` types?


================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:1055
+              ? LLT()
+              : (s == UINT64_C(0) ? LLT::opaque() : LLT::scalar(8 * s)),
+          a, AAInfo, Ranges, SSID, Ordering, FailureOrdering) {}
----------------
If this was the problematic piece of code, I'm surprised `LLT::pointer` isn't used anywhere here. My model for `opaque` as used here is that it is a zero-sized pointer rather than a zero-sized scalar. Am I thinking about it incorrectly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105423