[PATCH] D89083: [BPF] Make BPFAbstractMemberAccessPass required

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song added inline comments.


================
Comment at: llvm/lib/Target/BPF/BPF.h:50
+
+  static bool isRequired() { return true; }
 };
----------------
Could you explain what is the purpose of the above line?


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89083