[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
                                                   ImmArg<ArgIndex<2>>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+                                                   [llvm_ptr_ty],
----------------
yonghong-song wrote:
> eddyz87 wrote:
> > yonghong-song wrote:
> > > Is it possible to make this builtin as BPF specific one?
> > Currently `llvm::Intrinsic::context_marker_bpf` gets defined in `llvm/IR/Intrinsics.h` (via include of generated file `IntrinsicEnums.inc`, same as `preserve_{struct,union,array}_access_index`).
> > 
> > BPF specific intrinsics are defined in `llvm/IR/IntrinsicsBPF.h` (generated directly w/o .inc intermediary).
> > 
> > Thus, if I move `context_marker_bpf` to `IntrinsicsBPF.td` I would have to include `IntrinsicsBPF.h` in `CGExpr.cpp`. However, I don't see any target specific includes in that file.
> > 
> I went through the related clang code and indeed found it is hard to get a BPF target defined function in CGF or CGM. On the other hand, we can consider this new builtin under the umbrella "Intrinsics that are used to preserve debug information".  Maybe we can rename the intrinsic name
> to 'int_preserve_context_marker'? The goal of this builtin to preserve certain load/store which should be immune from optimizations. I try to generalize this so your function name 'wrapWithBPFContextMarker' can be renamed to
> 'wrapWithContextMarker'. There is no need to mention BPF any more.
> 
> In the commit message, you can mention that
> similar to int_preserve_array_access_index, int_preserve_context_marker is only implemented in BPF backend. But other architectures can implement processing these intrinsics if they want to achieve some results similar to bpf backend.
> 
> WDYT? 
I can rename these things, but tbh I don't think this functionality would be useful anywhere outside BPF, thus such renaming would be kind-of deceptive (and in case it would be useful, the renaming could be done at the time of second use).

---

Something more generic might look like `!btf_decl_tag <str-value>` metadata node attached to something. However, in the current form this would require transfer of such decl tag from type to function parameters and variables, e.g.:

```
lang=c
struct foo {  ... } __attribute__((btf_decl_tag("ctx")));
void bar(struct foo *p) { ... }
```

During code-gen for `bar` use rule like "if function parameter has type annotated with btf_decl_tag, attach metadata to such parameter":

```
lang=llvm
define void @bar(ptr %p !btf_decl_tag !1) { ... }
!1 = { "ctx" }
```

Such rule looks a bit weird:
- tag is transferred from type to it's usage
- what usages should be annotated? we care about function parameters but from generic point of view `alloca`s or field accesses should be annotated as well.

---

The same metadata approach but with "ctx" attributes annotating function parameters (as you suggested originally, if I recall correctly) seems to be most generic and least controversial of all, e.g.:

```
lang=c
void bar(struct foo *p __attribute__((btf_decl_tag("ctx")))) { ... }
```

Converted to:

```
lang=llvm
define void @bar(ptr %p !btf_decl_tag !1) { ... }
!1 = { "ctx" }
```

However, this is less ergonomic for BPF, because user will have to annotate function parameters. (On the other hand, no changes in the kernel headers would be necessary).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361