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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
