[BPF] Make BPFAbstractMemberAccessPass required

[aeubanks]

Summary

Or else on optnone functions we get the following during instruction selection:

fatal error: error in backend: Cannot select: intrinsic %llvm.preserve.struct.access.index

Currently the -O0 pipeline doesn't properly run passes registered via
TargetMachine::registerPassBuilderCallbacks(), so don't add that RUN
line yet. That will be fixed after this.

[yonghong-song]

llvm/lib/Target/BPF/BPF.h

50

Could you explain what is the purpose of the above line?

llvm/test/CodeGen/BPF/CORE/store-addr-optnone.ll

1 ↗

(On Diff #297081)

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.

[yonghong-song]

If I understand correctly, "isRequired()" means this pass has to run. In this regard, BPFPreserveDIType also need isRequired().
Could you explain a little bit what if one pass does not have "isRequired()"? How PassManager deals with it?

[aeubanks]

also make BPFPreserveDITypePass required and add test

[aeubanks]

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

llvm/test/CodeGen/BPF/CORE/store-addr-optnone.ll

1 ↗

(On Diff #297081)

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.

[yonghong-song]

Thanks for explanation for IsRequired(), really appreciated.

Could you change optnone-2.ll with the following source:
-bash-4.4$ cat t.c
struct ss { int a; };
int foo() { return builtin_btf_type_id(0, 0) + builtin_preserve_type_info(*(struct ss *)0, 0); }
-bash-4.4$

This will cover both BPFAbstractMemberAccess.cpp and BPFPreserveDIType.cpp. This way, we do not need store-addr-optnone.ll.

[aeubanks]

update test in accordance with comments

[yonghong-song]

LGTM. Thanks!

llvm/lib/Target/BPF/BPF.h

 
 class BPFAbstractMemberAccessPass
     : public PassInfoMixin<BPFAbstractMemberAccessPass> {
   BPFTargetMachine *TM;
 
 public:
   BPFAbstractMemberAccessPass(BPFTargetMachine *TM) : TM(TM) {}
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+
+  static bool isRequired() { return true; }
 };
 
 class BPFPreserveDITypePass : public PassInfoMixin<BPFPreserveDITypePass> {
 public:
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+
+  static bool isRequired() { return true; }
 };
 } // namespace llvm
 
 #endif

llvm/test/CodeGen/BPF/optnone-2.ll

+; RUN: opt < %s -passes='default<O2>' | llc -march=bpfel -filetype=asm -o /dev/null -
+; TODO: add -O0 once that's supported
+
+; IR generated by
+; $ cat /tmp/a.c
+; struct ss { int a; };
+; int foo() { return __builtin_btf_type_id(0, 0) + __builtin_preserve_type_info(*(struct ss *)0, 0); }
+; $ clang -target bpf -g -S -emit-llvm t.c -Xclang -disable-llvm-passes /tmp/a.c
+
+target triple = "bpf"
+
+; Function Attrs: noinline nounwind optnone
+define dso_local i32 @foo() #0 !dbg !9 {
+entry:
+  %0 = call i32 @llvm.bpf.btf.type.id(i32 0, i64 0), !dbg !12, !llvm.preserve.access.index !4
+  %1 = call i32 @llvm.bpf.preserve.type.info(i32 1, i64 0), !dbg !13, !llvm.preserve.access.index !14
+  %add = add i32 %0, %1, !dbg !17
+  ret i32 %add, !dbg !18
+}
+
+; Function Attrs: nounwind readnone
+declare i32 @llvm.bpf.btf.type.id(i32, i64) #1
+
+; Function Attrs: nounwind readnone
+declare i32 @llvm.bpf.preserve.type.info(i32, i64) #1
+
+attributes #0 = { noinline nounwind optnone }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "C:/src/tmp\\a.c", directory: "C:\\src\\llvm-project")
+!2 = !{}
+!3 = !{!4}
+!4 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!5 = !{i32 7, !"Dwarf Version", i32 4}
+!6 = !{i32 2, !"Debug Info Version", i32 3}
+!7 = !{i32 1, !"wchar_size", i32 4}
+!8 = !{!"clang version 12.0.0"}
+!9 = distinct !DISubprogram(name: "foo", scope: !10, file: !10, line: 2, type: !11, scopeLine: 2, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!10 = !DIFile(filename: "C:/src/tmp/a.c", directory: "")
+!11 = !DISubroutineType(types: !3)
+!12 = !DILocation(line: 2, column: 21, scope: !9)
+!13 = !DILocation(line: 2, column: 51, scope: !9)
+!14 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "ss", file: !10, line: 1, size: 32, elements: !15)
+!15 = !{!16}
+!16 = !DIDerivedType(tag: DW_TAG_member, name: "a", scope: !14, file: !10, line: 1, baseType: !4, size: 32)
+!17 = !DILocation(line: 2, column: 49, scope: !9)
+!18 = !DILocation(line: 2, column: 14, scope: !9)