[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D152986#4425736 <https://reviews.llvm.org/D152986#4425736>, @rnk wrote:

> 

Thank you for the review!

> The purpose of the attribute is really limited to preserving source location information on instructions, and this isn't really a supported usage.

But it would prevent merging, right? I understand that using this attribute might block some legitimate optimizations.
Or do you mean that it is a "best effort" thing and not all transformations might honor it?

> The BPF backend and verifier needs to learn to tolerate valid LLVM transforms if it wants to be a real LLVM backend. Of course, you can do what you like.

Well, I agree with that. The issue here is with the map API design: it is polymorphic, but it is expected that at load time verifier can replace all polymorphic calls with static calls.
People know it and write code in a way that allows verifier to infer which static functions to call. So I have limited options here:

- either adjust IR at early stages of pipeline so that specific calls are not altered;
- or do nothing and recommend users to use `[[clang::nomerge]]` on statement level or insert something like `asm volatile ("" :::"memory")` here and there.

But, yeah, I understand that is is not a C language semantics and headache is mine.

> Considered in the context of the original use case, I think it's reasonable to allow the attribute on function pointers for the same reasons we allow it on function declarations. It makes it easy to work the attribute onto the direct call sites of the function without modifying tons of source code. However, I'd like to see clearer documentation on the limitations.

Please see my "inline" response.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:551-555
 calls to the specified function from merging. It has no effect on indirect 
 calls.
+
+``nomerge`` attribute can be specified for pointers to functions, all
+calls done through such pointer would be protected from merging.
----------------
rnk wrote:
> This statement of the attribute having "no effect on indirect calls" is slightly confusing now that we talk about function pointers immediately afterward. Can you please rework this a bit, and clarify that when applied to function pointers, the attribute only takes effect when the call target is directly the variable which carries the attribute? For example, this has no effect:
> ```
> void (*fp)() __attribute__((nomerge));
> void callit() {
>   auto tmp = fp;
>   tmp();
>   (*fp)(); // I think TargetDecl will be null in the code, tell me if I'm wrong
> }
> ```
I can make it more elaborate, would the text as below be fine?
Regarding TargetDecl value it is not null both times:
- `(VarDecl 'tmp' (ImplicitCastExpr (DeclRefExpr (Var fp))))`
- `(VarDecl 'fp')`

---

``nomerge`` attribute can also be used as function attribute to prevent all 
calls to the specified function from merging. It has no effect on indirect 
calls to such functions. For example:

.. code-block:: c++

  [[clang::nomerge]] void foo(int) {}
  
  void bar(int x) {
    auto *ptr = foo;
    if (x) foo(1); else foo(2); // will not be merged
    if (x) ptr(1); else ptr(2); // indirect call, can be merged
  }

``nomerge`` attribute can also be used for pointers to functions to
prevent calls through such pointer from merging. In such case the
effect applies only to a specific function pointer. For example:

.. code-block:: c++

  [[clang::nomerge]] void (*foo)(int);
  
  void bar(int x) {
    auto *ptr = foo;
    if (x) foo(1); else foo(2); // will not be merged
    if (x) ptr(1); else ptr(2); // 'ptr' has no 'nomerge' attribute,
                                // can be merged
  }



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152986