This is an archive of the discontinued LLVM Phabricator instance.

[BPF] fix incorrect type in BPFISelDAGToDAG readonly load optimization
ClosedPublic

Authored by yonghong-song on Jun 10 2020, 10:24 PM.

Details

Summary

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.

Diff Detail

Event Timeline

yonghong-song created this revision.Jun 10 2020, 10:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 10:24 PM

Martin from Facebook reported this problem.

ast accepted this revision.Jun 11 2020, 2:02 PM
This revision is now accepted and ready to land.Jun 11 2020, 2:02 PM
This revision was automatically updated to reflect the committed changes.