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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

I will try to review other parts of code in the next few days.



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
                                                   ImmArg<ArgIndex<2>>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+                                                   [llvm_ptr_ty],
----------------
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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361