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

[Paulo Matos via Phabricator via llvm-commits]

pmatos added inline comments.


================
Comment at: llvm/include/llvm/Support/LowLevelTypeImpl.h:124
 
-  bool isValid() const { return RawData != 0; }
+  bool isValid() const { return IsScalar || RawData != 0; }
 
----------------
tlively wrote:
> Are pointers or vectors with `RawData == 0` invalid as well?
Yes, before `RawData == 0` meant invalid. However, now given we allow `RawData` to be zero for scalars, we added a bit `IsScalar` to mark scalars and for those allow `RawData` to be zero. So, to be valid, you're either a scalar (`IsScalar == 1`) or your raw data is nonzero (`RawData != 0`).


================
Comment at: llvm/include/llvm/Support/LowLevelTypeImpl.h:127
+  bool isScalar() const {
+    return isValid() && IsScalar && !IsPointer && !IsVector;
+  }
----------------
tlively wrote:
> Why isn't it sufficient to do `return IsScalar`? If `isScalar()` and `IsScalar` have different meanings, could that be clarified in the names?
It's true that you could just do `isValid() && IsScalar`, however the other checks are extra to ensure the type was correctly built. Since you could in principle create an LLT where all the bits are 1 (`IsScalar && IsPointer && IsVector`), this check ensures that a true scalar doesn't have those. On the other hand, this check probably belongs elsewhere. Maybe in `init`... I will change this. 


================
Comment at: llvm/include/llvm/Support/LowLevelTypeImpl.h:334
   uint64_t IsVector : 1;
-  uint64_t RawData : 62;
+  uint64_t RawData : 61;
 
----------------
arsenm wrote:
> Leftover change?
No, the RawData uses 1 less bit, so 61 instead of 62 because we are taking a bit to encode if a value is a scalar. We need this explicit bit because before a scalar was something with `!IsPointer && !IsVector`. However, by allowing `RawData` to be zero in scalar, we need a bit for scalars to verify that a value is a scalar and allow its rawdata to be zero.


================
Comment at: llvm/unittests/CodeGen/LowLevelTypeTest.cpp:24
   DataLayout DL("");
 
+  for (unsigned S : {0U, 1U, 17U, 32U, 64U, 0xfffffU}) {
----------------
arsenm wrote:
> I think we're missing an EXPECT_FALSE(LLT().isValid()) test
Where do you expect this?


================
Comment at: llvm/unittests/CodeGen/LowLevelTypeTest.cpp:54
   DataLayout DL("");
 
   for (unsigned S : {1U, 17U, 32U, 64U, 0xfffU}) {
----------------
arsenm wrote:
> What happens for a vector of opaque?
Good point, I hadn't thought of those. I will add a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105423