[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D32210#819577, @ahatanak wrote:

> In https://reviews.llvm.org/D32210#810508, @rjmccall wrote:
>
> > Hmm.  Unfortunately, I'm not sure that's valid.  The retains and releases of block captures don't protect against anything related to escaping the block; they protect against the original variables being modified during the lifetime of the block.  It is true that a block literal passed to a noescape parameter has a shorter effective lifetime — we know for certain that it will be unused after the call, and ARC only promises that a value in an imprecise-lifetime strong variable like a block capture will be valid until the last load of that value from the variable.  But that duration is still long enough for someone to modify the original variable in a way that is properly sequenced after the formation of the block.
> >
> > Now, if you could prove that the variable was not modified for the duration of the call, that would be sufficient.  And that would be easy to do in the common case by just proving that the address of the variable never escapes.  Unfortunately, we don't have that information readily available because Sema doesn't collect it.
>
>
> OK, so I guess the optimization isn't valid in the following case, for example:
>
>   void foo3(id);
>  
>   id __strong *gp;
>  
>   void foo2(id a) {
>     gp = &a;
>     nonescapingFunc(^{ foo3(a); }); // this function can modify "a" before the block is executed.
>   }
>


Right.

>> There are some other ways you could optimize blocks that are known not to escape, though.  One big caveat is that you have to make sure the block behaves reasonably in response to being copied, becase being noescape doesn't guarantee that the callee won't try to copy the block.  However:
> 
> I didn't understand the following optimizations. I thought clang doesn't emit copy and destroy helper for global blocks with or without noescape?

They're not really about global blocks, they're about whether we can emit a non-escaping block using the isa that we use for global blocks, which causes copies to be no-ops (because real global blocks do not have local captures).

John.



================
Comment at: include/clang/AST/Type.h:3156
     };
     unsigned char Data;
 
----------------
ahatanak wrote:
> rjmccall wrote:
> > Oh!  I hadn't noticed that you were adding this to ExtParameterInfo.  You're modifying the function type system; there's a lot of complexity to do that properly which you haven't done in this patch at all.  That's especially true because, unlike all these other ExtParameterInfo cases, there's a natural subtyping rule for escaping parameters: a function type that takes a non-escaping parameter should be implicitly convertible to be a function type that takes an escaping parameter.  You will also need to update a bunch of things, including the type printer, the mangler, the C++ function conversion rules, the C type compatibility rules, the redeclaration type matcher, and maybe even template argument deduction.  It's a big thing to do.
> > 
> > You will want Sema tests in C, ObjC, C++, and ObjC++.
> Rather than adding an enum to ExtParameterInfo and modifying the type system, I made changes in IRGen to find out whether a function parameter is annotated with noescape. I'm not sure when it is OK or not OK to change the type system when adding support for an attribute, but this seemed to be the right direction since most of the other attributes are handled this way.
So, I do think it's acceptable to change the type system here; it's just that doing so definitely makes the change more complicated.

The guidance here is:
  - If it changes the basic rules for making a call, it really *must* be preserved as part of the function type, or else basic things like taking the address of the function will never work correctly.  That's why everything currently in ExtParameterInfo is there — ns_consumed affects the balancing rules for how ARC makes calls, the Swift parameter-ABI treatments are part of the CC, etc.
  - Otherwise, it really comes down to how much we care about the feature working with indirection.  I feel like blocks are probably the strongest motivator here because they're always invoked indirectly: if you want the feature to work on a block parameter, it really needs to be part of the function type, because we do not want to get into the business of finding non-canonical sources of information for function types, e.g. param decls from the TypeSourceInfo.

Overall, I would say that it tends to be the case that we eventually find attribute-based analyses limiting.  If we start doing serious optimizations based on noescape, it is probably something that ought to go in the type system.


================
Comment at: lib/CodeGen/CGCall.cpp:2096
+    if (FI.getExtParameterInfo(ArgNo).isNoEscape())
+      Attrs.addAttribute(llvm::Attribute::NoCapture);
+
----------------
ahatanak wrote:
> rjmccall wrote:
> > You should make sure that building a CGFunctionInfo with an ObjCMethodDecl* collects this from the parameters.  (And add tests!)
> > 
> > For blocks, at least, the ObjC method case is arguably more important than the C function case.
> Should we issue a warning when a parameter of an ObjC method is annotated with noescape and the corresponding parameter of the overriding method in a derived class isn't? Also, should we warn about inconsistent C++ virtual functions too?
Mmm.  Probably yes in both cases, although we could alternatively just silently propagate the attribute InheritableParamAttr-style.


================
Comment at: lib/CodeGen/CGClass.cpp:2086
   CallArg ThisArg(RValue::get(This.getPointer()), D->getThisType(getContext()),
-                  /*NeedsCopy=*/false);
+                  /*NeedsCopy=*/false, /*IsNoEscapse*/false);
 
----------------
Typo.


https://reviews.llvm.org/D32210