[PATCH] D155894: [BPF] allow external calls

[Tamir Duberstein via Phabricator via llvm-commits]

tamird added inline comments.


================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:466
     Callee = DAG.getTargetExternalSymbol(E->getSymbol(), PtrVT, 0);
-    fail(CLI.DL, DAG, Twine("A call to built-in function '"
-                            + StringRef(E->getSymbol())
----------------
ast wrote:
> tamird wrote:
> > ast wrote:
> > > How does it help aya?
> > > 
> > > In general I think it's ok to remove this, but we need to define the behavior more accurately here.
> > > Does it mean that we promote builtin to global ? What is the visibility of such symbol? What relocation do we use ? What libbpf and libbpf linker should do with it?
> > > 
> > > In particular with memcpy, do we generate asm that calls "llvm.memcpy.p0.p0.i64" name or does it become plain "memcpy" ?
> > > How does it help aya?
> > 
> > In aya, we provide implementations of these built-ins (memset, memcpy, etc.). In bpf-linker, we combine all the IR into a single module before codegen. Currently this error fires, which we must treat as fatal or ignore by string matching.
> > 
> > It turns out that codegen generates a call to plain "memset", and the resulting code is completely valid. So this error is spurious.
> > 
> > > In general I think it's ok to remove this, but we need to define the behavior more accurately here.
> > > Does it mean that we promote builtin to global ? What is the visibility of such symbol? What relocation do we use ? What libbpf and libbpf linker should do with it?
> > 
> > Looks like yonghong discovered that this symbol is indeed promoted to global.
> > 
> > > 
> > > In particular with memcpy, do we generate asm that calls "llvm.memcpy.p0.p0.i64" name or does it become plain "memcpy" ?
> > 
> > Yes, it becomes "plain".
> Since backend couldn't unroll memset it should error hard.
> The linking step doesn't exist most of the time.
> From the user perspective it's better to fail at compile time instead of program loading time where the verifier errors are obscure.
> I think it would be good to always unroll memset into a loop even when it's huge.
> And similar with memcpy.
> -fno-builtin and __attribute__((no_builtin)) tricks are unrelated to any of this.
> 
Right. The `no_builtin` tricks are too big of hammers for what we want here, which is to still apply constant size optimizations but not apply special treatment to these functions when they've been defined by the user, which is what @yonghong-song was saying in his earlier comments. I may dig into that when I have some time, but for now we're just ignoring these errors in aya's linker.

Is there a place (issue tracker, mailing list, other) to document these musings and describe possible future work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155894