This is an archive of the discontinued LLVM Phabricator instance.

[IR] Teach `llvm::User` to co-allocate a descriptor.
ClosedPublic

Authored by sanjoy on Aug 28 2015, 4:22 PM.

Details

Summary

With this change, subclasses of llvm::User will be able to co-allocate
a variable number of bytes (called a "descriptor") with the llvm::User
instance. The co-allocated descriptor can later be accessed using
llvm::User::getDescriptor. This will be used in later changes to
implement operand bundles.

This change steals one bit from NumUserOperands, but given that it is
still 28 bits wide I don't think this will be a practical issue.

This change does not allow allocating hung off uses with descriptors.
This only for simplicity, not for any fundamental reason; and we can
easily add this functionality later if needed.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 33487.Aug 28 2015, 4:22 PM
sanjoy retitled this revision from to [IR] Teach `llvm::User` to co-allocate a descriptor..
sanjoy updated this object.
sanjoy added a subscriber: llvm-commits.
include/llvm/IR/User.h
49–54 ↗(On Diff #33487)

I wonder what sort of alignment guarantees this should make / what sort of alignment expectations it should set. The end of this allocation will always be 4 bytes before a pointer-aligned address, correct? Maybe just note that in the comment so that callers can figure out how to get pointer alignment if they want it?

sanjoy added inline comments.
include/llvm/IR/User.h
49–54 ↗(On Diff #33487)

Replied in D12456 (basically I'll move the assertions here, and add a top level comment).

sanjoy added a subscriber: sanjoy.Aug 31 2015, 9:31 PM

I don't know why this hasn't showed up on llvm-commits yet, but I've
replied to the review on phabricator.

sanjoy updated this revision to Diff 33748.Sep 1 2015, 4:06 PM

Address review.

Looks good to me (not that I'm an expert on this code...), thanks for the update.

pete added a subscriber: pete.Sep 2 2015, 11:22 AM

This is just a question, not intended to block the commit if someone else gives a LGTM.

Have you considered making use of 'hung off uses' instead of adding DescBytes to new?

It would take a little work to change 'hung off uses' to actually be 'hung off bytes', and i haven't looked to see how possible that is, but it feels to me like it should be considered.

This also has the advantage of moving the desc bytes handling out of new so that all User's don't pay the price for checking them in new, and instead the rare case of meeting extra bytes can get them only on request.

Just my 2c though.

Cheers,
Pete

sanjoy added a comment.Sep 2 2015, 2:00 PM
In D12455#238526, @pete wrote:

This is just a question, not intended to block the commit if someone else gives a LGTM.

Have you considered making use of 'hung off uses' instead of adding DescBytes to new?

It would take a little work to change 'hung off uses' to actually be 'hung off bytes', and i haven't looked to see how possible that is, but it feels to me like it should be considered.

This also has the advantage of moving the desc bytes handling out of new so that all User's don't pay the price for checking them in new, and instead the rare case of meeting extra bytes can get them only on request.

Is the only motivation for making the descriptor bytes be hung off
that we won't have to check DescriptorBytes == 0 in operator new
(I cannot think of anything else that hung off descriptor bytes will
help)?

If so, I think there is an easier way to fix this -- I can just
overload operator new into a version that takes the
DescriptorBytes argument, and a version that does not. Most of
Users will call the latter, and won't have to pay the penalty of
checking DescriptorBytes == 0. There will still be the penalty of
setting Obj->HasDescriptor = 0 in the latter, but I'd expect that to
be folded into whatever operator new does to set
Obj->HasHungOffUses = false. Does this sound reasonable to you?

pete added a comment.Sep 2 2015, 2:03 PM
In D12455#238526, @pete wrote:

This is just a question, not intended to block the commit if someone else gives a LGTM.

Have you considered making use of 'hung off uses' instead of adding DescBytes to new?

It would take a little work to change 'hung off uses' to actually be 'hung off bytes', and i haven't looked to see how possible that is, but it feels to me like it should be considered.

This also has the advantage of moving the desc bytes handling out of new so that all User's don't pay the price for checking them in new, and instead the rare case of meeting extra bytes can get them only on request.

Is the only motivation for making the descriptor bytes be hung off
that we won't have to check DescriptorBytes == 0 in operator new
(I cannot think of anything else that hung off descriptor bytes will
help)?

If so, I think there is an easier way to fix this -- I can just
overload operator new into a version that takes the
DescriptorBytes argument, and a version that does not. Most of
Users will call the latter, and won't have to pay the penalty of
checking DescriptorBytes == 0. There will still be the penalty of
setting Obj->HasDescriptor = 0 in the latter, but I'd expect that to
be folded into whatever operator new does to set
Obj->HasHungOffUses = false. Does this sound reasonable to you?

Yeah, that was the only motivation. Like I said, shouldn't block things if you think it'll be ok.

Saying that, I do prefer your suggestion to overload operator new. I'd be totally fine with that.

Thanks!

pete accepted this revision.Sep 2 2015, 2:05 PM
pete added a reviewer: pete.

Just noticed there was a LGTM already.

FWIW, i'm also happy to give a LGTM with/without the overloaded operator new. Up to you how you proceed on that.

This revision is now accepted and ready to land.Sep 2 2015, 2:05 PM
sanjoy updated this revision to Diff 33864.Sep 2 2015, 3:18 PM
sanjoy edited edge metadata.
  • specialize operator new (addressing @pete's review comment)
  • address @majnemer's off-line review comment -- return an ArrayRef and not an std::pair
sanjoy added a comment.Sep 2 2015, 3:20 PM

@pete - can you please take a look to see if you're okay with how I've specialized operator new?

sanjoy updated this revision to Diff 34749.Sep 14 2015, 3:53 PM
  • add inline keyword to allocateFixedOperandUser
This revision was automatically updated to reflect the committed changes.