Page MenuHomePhabricator

Add support for zero-sized Scalars as a LowLevelType
ClosedPublic

Authored by pmatos on Jul 5 2021, 2:48 AM.

Details

Summary

Opaque values (of zero size) can be stored in memory with the implemention of
reference types in the WebAssembly backend. Since MachineMemOperand
uses LLTs we need to be able to support zero-sized scalars types in LLTs.

Diff Detail

Event Timeline

pmatos created this revision.Jul 5 2021, 2:48 AM
pmatos requested review of this revision.Jul 5 2021, 2:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2021, 2:48 AM
pmatos added a subscriber: arsenm.

Adding @arsenm as reviewer since he committed https://github.com/llvm/llvm-project/commit/990278d026d680942c859be70836ad34a9a716f7 , which broke d104797 and forced this patch to be required.

pmatos added a subscriber: sdesmalen.

Adding @sdesmalen as reviewer, who was one of the last authors of changes to LLT implementation.

pmatos updated this revision to Diff 356931.Jul 7 2021, 6:06 AM

Apply clang-format changes

arsenm added a comment.Jul 7 2021, 6:13 AM

I'm not sure what an opaque type means. Do you have an IR sample?

pmatos added a comment.Jul 7 2021, 8:14 AM

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
}
arsenm added a comment.Jul 7 2021, 8:17 AM

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?

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?

llvm/include/llvm/Support/LowLevelTypeImpl.h
280–281

Why did the number of elements change? What is the third element for? It would be good to update the comment.

284

How are the bitfields packed for opaque types?

llvm/lib/CodeGen/MachineOperand.cpp
1058

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?

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?

Sorry @arsenm , I should have explained it better, as @tlively did instead of posting the example and leaving for the day.

In any case, as @tlively said the MVT for externref is zero-sized. See https://github.com/llvm/llvm-project/blob/0fd5e7b2d8ca4ed46d76187feb4b903ed0a3ea75/llvm/include/llvm/Support/MachineValueType.h#L1049

@tlively actually your suggestion of making the opaque type a scalar type of size zero was my first attempt : Failed! Why? The idea what that I would differentiate between a valid scalar and an opaque type depending on if its value was zero. So, I actually implemented an initial patch where I had something like this:

bool IsOpaque() { return isValid() && !IsPointer && !IsVector && getSizeInBits() == 0; }
bool IsScalar() { return isValid() && !IsPointer && !IsVector && getSizeInBits() != 0; }

I also created a constructor to construct an opaque type as a scalar type of size zero. But this didn't work because the implementation of getSizeInBits() itself calls IsScalar(). So this resulted in an infinite recursive call. I then decided to create its own type for Opaque as in this patch.

However, I am looking at the code again and notice that actually, I could call in IsOpaque() the function getScalarSizeInBits() instead of getSizeInBits(). That could work. I will get this patch redone, test, and submit it as an alternative revision.

pmatos added inline comments.Jul 8 2021, 12:13 AM
llvm/lib/CodeGen/MachineOperand.cpp
1058

To be honest I was quite surprise as well when looked at it. I initially thought LLT::scalar(8 * s) should be a pointer instead. However, @arsenm added a call to the scalar constructor instead, and I followed the lead for opaque. My understanding here atm for the use of LLT::scalar for pointers is that this is the type representation of a pointer in memory - a scalar of a certain size, and not the representation of the value pointed to.

pmatos added inline comments.Jul 8 2021, 12:15 AM
llvm/include/llvm/Support/LowLevelTypeImpl.h
280–281

I misunderstood the point of this array. Reverting the change. Thanks.

pmatos updated this revision to Diff 357170.Jul 8 2021, 2:14 AM

Change representation of opaque types as scalars of size zero

pmatos updated this revision to Diff 357185.Jul 8 2021, 3:50 AM

Remove extraneous code

pmatos updated this revision to Diff 357190.Jul 8 2021, 4:19 AM

Remove conditional on zero size

pmatos added a comment.Jul 8 2021, 4:23 AM

OK - I feel like this became an easier diff to deal with by doing opaques as scalar of size 0. However, to be honest I am not totally convinced yet it's useful to introduce the idea of "opaque" here. Just like we create MVT's that are Fixed(0) maybe we should just allow LLT's to have scalar(0) calls and remove this "opaque" concept here. What do others think?

arsenm added a comment.Jul 8 2021, 6:53 AM

OK - I feel like this became an easier diff to deal with by doing opaques as scalar of size 0. However, to be honest I am not totally convinced yet it's useful to introduce the idea of "opaque" here. Just like we create MVT's that are Fixed(0) maybe we should just allow LLT's to have scalar(0) calls and remove this "opaque" concept here. What do others think?

That would be a bit nicer than taking a bit just for this. However some places are relying on invalid LLTs that are all zeroes currently

Nice! This is a lot simpler. I agree that it would be nicer to avoid introducing a new opaque concept here entirely, but if fixing up users that depend on zero-sized LLTs to be invalid looks tricky, then this solution seems reasonable.

pmatos added a comment.Jul 9 2021, 2:37 AM

Nice! This is a lot simpler. I agree that it would be nicer to avoid introducing a new opaque concept here entirely, but if fixing up users that depend on zero-sized LLTs to be invalid looks tricky, then this solution seems reasonable.

My idea what to just remove the idea of opaque. I think that a scalar 0 is different from

Nice! This is a lot simpler. I agree that it would be nicer to avoid introducing a new opaque concept here entirely, but if fixing up users that depend on zero-sized LLTs to be invalid looks tricky, then this solution seems reasonable.

Actually currently the solution is marking RawData as 0 for opaque types without any further special handling so it's actually invalid for those calling LLT::isValid().
I can remove the concept of opaque and allow 0 sized scalars, but will need a bit for IsScalar. Will give that a go.

An update that this is taking a while because allowing the representation of zero-sized scalars breaks quite a few things. Fixing these have been a slow process, but I hope to have a proposal soon.

pmatos updated this revision to Diff 358932.Jul 15 2021, 5:30 AM

Remove reference to opaque type and allow scalars of zero size.

As a consequence, not all scalar LLTs can now be converted to IR Integer Types.

pmatos updated this revision to Diff 358937.Jul 15 2021, 5:39 AM

No changes to MachineOperand are necessary, reformatting to remove diff.

@arsenm @tlively what does this look like to you now?

pmatos updated this revision to Diff 358962.Jul 15 2021, 7:44 AM

Apply clang-format changes

Looks good beyond these clarification questions! It would also be good to update the revision title and description.

llvm/include/llvm/Support/LowLevelTypeImpl.h
123

Are pointers or vectors with RawData == 0 invalid as well?

126

Why isn't it sufficient to do return IsScalar? If isScalar() and IsScalar have different meanings, could that be clarified in the names?

arsenm added inline comments.Jul 19 2021, 5:30 PM
llvm/include/llvm/Support/LowLevelTypeImpl.h
332

Leftover change?

llvm/unittests/CodeGen/LowLevelTypeTest.cpp
24 ↗(On Diff #358962)

I think we're missing an EXPECT_FALSE(LLT().isValid()) test

49 ↗(On Diff #358962)

What happens for a vector of opaque?

54 ↗(On Diff #358962)

What happens for a vector of opaque?

pmatos added inline comments.Jul 20 2021, 2:24 AM
llvm/include/llvm/Support/LowLevelTypeImpl.h
123

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

126

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.

332

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.

llvm/unittests/CodeGen/LowLevelTypeTest.cpp
24 ↗(On Diff #358962)

Where do you expect this?

54 ↗(On Diff #358962)

Good point, I hadn't thought of those. I will add a test.

pmatos added inline comments.Jul 20 2021, 2:34 AM
llvm/unittests/CodeGen/LowLevelTypeTest.cpp
24 ↗(On Diff #358962)

Ah, is this what you mean:

TEST(LowLevelTypeTest, Invalid) {
  const LLT Ty;

  ASSERT_FALSE(Ty.isValid());

Line 291 of this file.

pmatos updated this revision to Diff 360073.Jul 20 2021, 3:41 AM
pmatos retitled this revision from Add support for Opaque as a LowLevelType to Add support for zero-sized Scalars as a LowLevelType.
pmatos edited the summary of this revision. (Show Details)

Update revision title and summary. Allow zero-sized scalars in vectors.

pmatos added inline comments.Jul 20 2021, 3:46 AM
llvm/include/llvm/Support/LowLevelTypeImpl.h
332

I am starting to think we could actually get away without a IsScalar, and allow a !IsPointer && !IsVector of zero size. There's a reason I added an IsScalar but I forget exactly why. I will do some further investigation.

pmatos added inline comments.Jul 21 2021, 7:30 AM
llvm/include/llvm/Support/LowLevelTypeImpl.h
332

Argh, no. Of course we can't. Because the invalid LLT is represented by all zeros, if we allow scalar to be a zero, then we need to have a bit for scalars so that the zero scalar is not marked incorrectly as invalid.

pmatos updated this revision to Diff 360468.Jul 21 2021, 8:12 AM

Tidy up isScalar() and improve code formatting

pmatos marked 2 inline comments as done.Jul 21 2021, 8:15 AM
pmatos marked 5 inline comments as done.

@tlively @arsenm I hope to have addressed all the pending issues.

tlively accepted this revision.Jul 21 2021, 5:08 PM

LGTM with that final simplification.

llvm/include/llvm/Support/LowLevelTypeImpl.h
126

Now this can be simplified to return IsScalar because isValid() is true if IsScalar is true.

This revision is now accepted and ready to land.Jul 21 2021, 5:08 PM
This revision was landed with ongoing or failed builds.Jul 22 2021, 4:48 AM
This revision was automatically updated to reflect the committed changes.