[PATCH] D89083: [BPF] Make BPFAbstractMemberAccessPass required

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D89083#2320798 <https://reviews.llvm.org/D89083#2320798>, @yonghong-song wrote:

> If I understand correctly, "isRequired()" means this pass has to run. In this regard, `BPFPreserveDIType` also need `isRequired()`.

Ah I didn't know both were required. I added that and added your test case. Your test case seems to only cover `BPFPreserveDITypePass`, the one already added here covers `BPFAbstractMemberAccessPass`.

> Could you explain a little bit what if one pass does not have "isRequired()"? How PassManager deals with it?

http://llvm.org/docs/WritingAnLLVMNewPMPass.html#required-passes has some explanation.
If a pass doesn't have `isRequired() { return true; }`, pass managers will skip it if something says to skip optional passes, like `optnone`.



================
Comment at: llvm/test/CodeGen/BPF/CORE/store-addr-optnone.ll:1
+; RUN: opt -passes='default<O2>' %s | llvm-dis > %t1
+; RUN: llc -filetype=asm -o - %t1 | FileCheck %s
----------------
yonghong-song wrote:
> Currently, for BPF ecosystem, we only recommend -O2, and all our major CI tests and kernel self tests are all compiled with -O2. It is known that -O0 (optnone) does not work for bpf programs as it may pass compilation, but it won't pass kernel verifier, and we have not extensively verified correctness of -O0 generated code either (esp. related to relocations). So I think this optnone thing is really a lower priority and it should not impct anyone.
> 
> But yes, even if -O0 is not recommended to developer and practically nobody uses it. But failing compilation still bad.  Currently BPF unit test only has one optnone test optnone-1.ll which did not check generated code except ensure it passes compilation. 
> 
> If you really want a test case, you can add an
> optnone-2.ll.
> 
> The code can be
> -bash-4.4$ cat t.c
> int foo() { return __builtin_btf_type_id(0, 0); }
> and to generate IR for opt:
> clang -target bpf -g -S -emit-llvm t.c -Xclang -disable-llvm-passes
> 
> And just to check whether compilation is successful or not.
> 
> In my opinion, NPM does not support optnone yet, I think you can add the test (if you want) later once the support is there. Adding this test won't add coverage right now.
NPM does support optnone as of https://reviews.llvm.org/D87869, we now just have to make sure that all required passes are marked so.

Yes I think compilation failing is quite bad. Currently BPF is the only target that adds required passes via `registerPassBuilderCallbacks`, but likely others will in the future. Currently all the -O0 pipeline (in clang and opt) don't run passes added by `registerPassBuilderCallbacks` and I'd like a test case once that's fixed. Currently the only test case I can come up with is BPF related tests, like the `opt -passes=default<O2>` in this patch, but instead `opt -passes=default<O0>`.

If the generated code from -O0 is not well tested, that's fine, again I'm just looking for a test case to show that we actually run passes added by 
`registerPassBuilderCallbacks` in all circumstances.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89083