[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

2

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

2

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/CORE/store-addr-optnone.ll

+; RUN: opt -passes='default<O2>' %s | llvm-dis > %t1
+; RUN: llc -filetype=asm -o - %t1 | FileCheck %s
+; RUN: llc -mattr=+alu32 -filetype=asm -o - %t1 | FileCheck %s
+; TODO: add -O0 RUN when it properly runs pipeline EP callbacks.
+
+; Source code:
+;   struct t {
+;     int a;
+;   } __attribute__((preserve_access_index));
+;   int foo(void *);
+;   int test(struct t *arg) {
+;       long param[1];
+;       param[0] = (long)&arg->a;
+;       return foo(param);
+;   }
+; Compiler flag to generate IR:
+;   clang -target bpf -S -O2 -g -emit-llvm -Xclang -disable-llvm-passes test.c
+
+target triple = "bpf"
+
+%struct.t = type { i32 }
+
+; Function Attrs: nounwind
+define dso_local i32 @test(%struct.t* %arg) local_unnamed_addr #0 !dbg !14 {
+entry:
+  %param = alloca [1 x i64], align 8
+  call void @llvm.dbg.value(metadata %struct.t* %arg, metadata !22, metadata !DIExpression()), !dbg !27
+  %0 = bitcast [1 x i64]* %param to i8*, !dbg !28
+  call void @llvm.lifetime.start.p0i8(i64 8, i8* nonnull %0) #5, !dbg !28
+  call void @llvm.dbg.declare(metadata [1 x i64]* %param, metadata !23, metadata !DIExpression()), !dbg !29
+  %1 = tail call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.ts(%struct.t* %arg, i32 0, i32 0), !dbg !30, !llvm.preserve.access.index !18
+  %2 = ptrtoint i32* %1 to i64, !dbg !31
+  %arrayidx = getelementptr inbounds [1 x i64], [1 x i64]* %param, i64 0, i64 0, !dbg !32
+  store i64 %2, i64* %arrayidx, align 8, !dbg !33, !tbaa !34
+  %call = call i32 @foo(i8* nonnull %0) #5, !dbg !38
+  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %0) #5, !dbg !39
+  ret i32 %call, !dbg !40
+}
+
+; CHECK:  r[[OFFSET:[0-9]+]] = 0
+; CHECK:  r1 += r[[OFFSET]]
+; CHECK:  *(u64 *)(r10 - 8) = r1
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture) #2
+
+; Function Attrs: nounwind readnone
+declare i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.ts(%struct.t*, i32, i32) #3
+
+declare !dbg !5 dso_local i32 @foo(i8*) local_unnamed_addr #4
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture) #2
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { optnone noinline nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable }
+attributes #2 = { argmemonly nounwind }
+attributes #3 = { nounwind readnone }
+attributes #4 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #5 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10, !11, !12}
+!llvm.ident = !{!13}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 (https://github.com/llvm/llvm-project.git 4f995959a05ae94cc4f9cc80035f7e4b3ecd2d88)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/tmp/home/yhs/work/tests/core")
+!2 = !{}
+!3 = !{!4, !5}
+!4 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+!5 = !DISubprogram(name: "foo", scope: !1, file: !1, line: 4, type: !6, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+!6 = !DISubroutineType(types: !7)
+!7 = !{!8, !9}
+!8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!10 = !{i32 7, !"Dwarf Version", i32 4}
+!11 = !{i32 2, !"Debug Info Version", i32 3}
+!12 = !{i32 1, !"wchar_size", i32 4}
+!13 = !{!"clang version 11.0.0 (https://github.com/llvm/llvm-project.git 4f995959a05ae94cc4f9cc80035f7e4b3ecd2d88)"}
+!14 = distinct !DISubprogram(name: "test", scope: !1, file: !1, line: 5, type: !15, scopeLine: 5, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !21)
+!15 = !DISubroutineType(types: !16)
+!16 = !{!8, !17}
+!17 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !18, size: 64)
+!18 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t", file: !1, line: 1, size: 32, elements: !19)
+!19 = !{!20}
+!20 = !DIDerivedType(tag: DW_TAG_member, name: "a", scope: !18, file: !1, line: 2, baseType: !8, size: 32)
+!21 = !{!22, !23}
+!22 = !DILocalVariable(name: "arg", arg: 1, scope: !14, file: !1, line: 5, type: !17)
+!23 = !DILocalVariable(name: "param", scope: !14, file: !1, line: 6, type: !24)
+!24 = !DICompositeType(tag: DW_TAG_array_type, baseType: !4, size: 64, elements: !25)
+!25 = !{!26}
+!26 = !DISubrange(count: 1)
+!27 = !DILocation(line: 0, scope: !14)
+!28 = !DILocation(line: 6, column: 5, scope: !14)
+!29 = !DILocation(line: 6, column: 10, scope: !14)
+!30 = !DILocation(line: 7, column: 28, scope: !14)
+!31 = !DILocation(line: 7, column: 16, scope: !14)
+!32 = !DILocation(line: 7, column: 5, scope: !14)
+!33 = !DILocation(line: 7, column: 14, scope: !14)
+!34 = !{!35, !35, i64 0}
+!35 = !{!"long", !36, i64 0}
+!36 = !{!"omnipotent char", !37, i64 0}
+!37 = !{!"Simple C/C++ TBAA"}
+!38 = !DILocation(line: 8, column: 12, scope: !14)
+!39 = !DILocation(line: 9, column: 1, scope: !14)
+!40 = !DILocation(line: 8, column: 5, scope: !14)

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
+; $ clang -target bpf -g -S -emit-llvm t.c -Xclang -disable-llvm-passes
+; `int foo() { return __builtin_btf_type_id(0, 0); }`
+
+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
+  ret i32 %0, !dbg !13
+}
+
+; Function Attrs: nounwind readnone
+declare i32 @llvm.bpf.btf.type.id(i32, i64) #1
+
+attributes #0 = { noinline nounwind optnone "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+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: 1, type: !11, scopeLine: 1, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!10 = !DIFile(filename: "C:/src/tmp/a.c", directory: "")
+!11 = !DISubroutineType(types: !3)
+!12 = !DILocation(line: 1, column: 20, scope: !9)
+!13 = !DILocation(line: 1, column: 13, scope: !9)