Skip to content

Commit f5bad9c

Browse files
yonghong-songtstellar
authored andcommitted
[BPF] fix incorrect type in BPFISelDAGToDAG readonly load optimization
In BPF Instruction Selection DAGToDAG transformation phase, BPF backend had an optimization to turn load from readonly data section to direct load of the values. This phase is implemented before libbpf has readonly section support and before alu32 is supported. This phase however may generate incorrect type when alu32 is enabled. The following is an example, -bash-4.4$ cat ~/tmp2/t.c struct t { unsigned char a; unsigned char b; unsigned char c; }; extern void foo(void *); int test() { struct t v = { .b = 2, }; foo(&v); return 0; } The compiler will turn local variable "v" into a readonly section. During instruction selection phase, the compiler generates two loads from readonly section, one 2 byte load or 1 byte load, e.g., for 2 loads, t8: i32,ch = load<(dereferenceable load 2 from `i8* getelementptr inbounds (%struct.t, %struct.t* @__const.test.v, i64 0, i32 0)`, align 1), anyext from i16> t3, GlobalAddress:i64<%struct.t* @__const.test.v> 0, undef:i64 t9: ch = store<(store 2 into %ir.v1.sub1), trunc to i16> t3, t8, FrameIndex:i64<0>, undef:i64 BPF backend changed t8 to i64 = Constant<2> and eventually the generated machine IR: t10: i64 = MOV_ri TargetConstant:i64<2> t40: i32 = SLL_ri_32 t10, TargetConstant:i32<8> t41: i32 = OR_ri_32 t40, TargetConstant:i64<0> t9: ch = STH32<Mem:(store 2 into %ir.v1.sub1)> t41, TargetFrameIndex:i64<0>, TargetConstant:i64<0>, t3 Note that t10 in the above is not correct. The type should be i32 and instruction should be MOV_ri_32. The reason for incorrect insn selection is BPF insn selection generated an i64 constant instead of an i32 constant as specified in the original load instruction. Such incorrect insn sequence eventually caused the following fatal error when a COPY insn tries to copy a 64bit register to a 32bit subregister. Impossible reg-to-reg copy UNREACHABLE executed at ../lib/Target/BPF/BPFInstrInfo.cpp:42! This patch fixed the issue by using the load result type instead of always i64 when doing readonly load optimization. Differential Revision: https://reviews.llvm.org/D81630 (cherry picked from commit 4db1878)
1 parent 0777c90 commit f5bad9c

File tree

2 files changed

+51
-1
lines changed

2 files changed

+51
-1
lines changed

llvm/lib/Target/BPF/BPFISelDAGToDAG.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ void BPFDAGToDAGISel::PreprocessLoad(SDNode *Node,
304304

305305
LLVM_DEBUG(dbgs() << "Replacing load of size " << size << " with constant "
306306
<< val << '\n');
307-
SDValue NVal = CurDAG->getConstant(val, DL, MVT::i64);
307+
SDValue NVal = CurDAG->getConstant(val, DL, LD->getValueType(0));
308308

309309
// After replacement, the current node is dead, we need to
310310
// go backward one step to make iterator still work

llvm/test/CodeGen/BPF/rodata_5.ll

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
; RUN: llc < %s -march=bpfel -mattr=+alu32 -verify-machineinstrs | FileCheck %s
2+
; RUN: llc < %s -march=bpfeb -mattr=+alu32 -verify-machineinstrs | FileCheck %s
3+
;
4+
; Source Code:
5+
; struct t {
6+
; unsigned char a;
7+
; unsigned char b;
8+
; unsigned char c;
9+
; };
10+
; extern void foo(void *);
11+
; int test() {
12+
; struct t v = {
13+
; .b = 2,
14+
; };
15+
; foo(&v);
16+
; return 0;
17+
; }
18+
; Compilation flag:
19+
; clang -target bpf -O2 -S -emit-llvm t.c
20+
21+
%struct.t = type { i8, i8, i8 }
22+
23+
@__const.test.v = private unnamed_addr constant %struct.t { i8 0, i8 2, i8 0 }, align 1
24+
25+
; Function Attrs: nounwind
26+
define dso_local i32 @test() local_unnamed_addr {
27+
entry:
28+
%v1 = alloca [3 x i8], align 1
29+
%v1.sub = getelementptr inbounds [3 x i8], [3 x i8]* %v1, i64 0, i64 0
30+
call void @llvm.lifetime.start.p0i8(i64 3, i8* nonnull %v1.sub)
31+
call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 1 dereferenceable(3) %v1.sub, i8* nonnull align 1 dereferenceable(3) getelementptr inbounds (%struct.t, %struct.t* @__const.test.v, i64 0, i32 0), i64 3, i1 false)
32+
call void @foo(i8* nonnull %v1.sub)
33+
call void @llvm.lifetime.end.p0i8(i64 3, i8* nonnull %v1.sub)
34+
ret i32 0
35+
}
36+
; CHECK-NOT: w{{[0-9]+}} = *(u16 *)
37+
; CHECK-NOT: w{{[0-9]+}} = *(u8 *)
38+
; CHECK: *(u16 *)(r10 - 4) = w{{[0-9]+}}
39+
; CHECK: *(u8 *)(r10 - 2) = w{{[0-9]+}}
40+
41+
; Function Attrs: argmemonly nounwind willreturn
42+
declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
43+
44+
; Function Attrs: argmemonly nounwind willreturn
45+
declare void @llvm.memcpy.p0i8.p0i8.i64(i8* noalias nocapture writeonly, i8* noalias nocapture readonly, i64, i1 immarg)
46+
47+
declare dso_local void @foo(i8*) local_unnamed_addr
48+
49+
; Function Attrs: argmemonly nounwind willreturn
50+
declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)

0 commit comments

Comments
 (0)