Page MenuHomePhabricator

OpaquePtr: add type to inalloca attribute.
AcceptedPublic

Authored by t.p.northover on Jan 7 2020, 6:07 AM.

Details

Summary

Similarly to byval, the inalloca aergument attribute (for MSVC compatibility) currently works by looking at the pointee type of its argument. This is no longer going to work with opaque pointers, and needs to be adapted reasonably early to allow any front-ends time to upgrade their usage.

So this patch adds an optional type, pretty much following byval's implementation directly from last year, so that part is probably pretty straightforward.

What might be worth thinking about is just where we enforce that byval and inalloca are incompatible. The situation I eneded up with was:

  • AttrBuilder allows you to create an AttributeSet with both (as before, but needed a new Type field to implement).
  • IR: they can exist together (as before)
  • Verifier: complains about IR in that state (as before).
  • CodeGen: shoves everything into a single "type" and "align" field. As before, but I've renamed them to be a bit more use-case agnostic in this patch.

I think that mostly makes sense, except possibly for the AttrBuilder. It might instead make sense to assert if someone tries to create a set with both.

Diff Detail

Event Timeline

t.p.northover created this revision.Jan 7 2020, 6:07 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
jdoerfert resigned from this revision.Jan 7 2020, 7:58 AM
sstefan1 resigned from this revision.Jan 7 2020, 10:50 AM
dblaikie accepted this revision.Jan 7 2020, 1:39 PM
dblaikie added a subscriber: rnk.

Looks pretty good to me - couple of minor things. If you want to check with @rnk (best point of contact on the inalloca feature in general), that'd probably be good.

Might've been a bit easier to review with the renamings in separate patches, but this makes it easier to see all the call sites, which is handy.

llvm/lib/IR/Attributes.cpp
885

Probably use nullptr, rather than 0 here.

llvm/lib/IR/Core.cpp
150–155

could drop the else-after-returns here, but I realize it's consistent with existing style, so maybe better in a separate pre/post patch

This revision is now accepted and ready to land.Jan 7 2020, 1:39 PM

I made the change suggested to Core.cpp style and rebased this patch, and fixed the nullptr issue David pointed out.

@rnk mind taking a look at this when you get a chance just to sanity check from the inalloca design side of things?

rnk accepted this revision.Jan 26 2020, 8:21 AM

lgtm, thanks!

I remember reading through this last week, but apparently I did not hit send. This patch actually prompted me to start working on the replacement for this thing.