[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Eduard Zingerman via Phabricator via llvm-commits]

eddyz87 added inline comments.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3791
+  if (hasBPFPreserveStaticOffset(Base))
+    addr = wrapWithBPFPreserveStaticOffset(CGF, addr);
+
----------------
ast wrote:
> eddyz87 wrote:
> > ast wrote:
> > > eddyz87 wrote:
> > > > ast wrote:
> > > > > If I'm reading this correctly wrapping with preserve_static_offset doesn't prevent further preserver_access_index wrapping which is a wasted effort for pai at the end ?
> > > > Yes, pai calls are undone in `BPFPreserveStaticOffset.cpp:removePAICalls()`. I can put back the logic that suppresses pai if preserve static offset is present.
> > > I see. I guess I missed a previous discussion. Why this approach was chosen?
> > Initial version used `__attribute__((btf_decl_tag("ctx")))` and Yonghong did not want to have prioritization between `btf_decl_tag` and `preserve_access_index` basing on decl tag string parameter. Now this limitation is gone (and I think this was one of your arguments in favor of separate attribute).
> Ahh. Right. It made sense to avoid special treatment of strings in decl_tag, but now it's gone and PSO takes precedence over PAI. Here we're adding PAI just to remove it later. Looks like a waste of cpu cycles and code. Unless applying PAI to outer struct and PSO in inner makes implementation tricky. I doubt we need to support such combo though. I'm fine cleaning this up in a follow up. If such cleanup makes sense at all.
> If such cleanup makes sense at all.

I think it does, I'll implement this change and push an update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361