This is an archive of the discontinued LLVM Phabricator instance.

BPF: simplify IR generation for __builtin_btf_type_id()
ClosedPublic

Authored by yonghong-song on Aug 3 2020, 5:17 PM.

Details

Summary

This patch simplified IR generation for builtin_btf_type_id().
For
builtin_btf_type_id(obj, flag), previously IR builtin
looks like

if (obj is a lvalue)
  llvm.bpf.btf.type.id(obj.ptr, 1, flag)  !type
else
  llvm.bpf.btf.type.id(obj, 0, flag)  !type

The purpose of the 2nd argument is to differentiate

__builtin_btf_type_id(obj, flag) where obj is a lvalue

vs.

__builtin_btf_type_id(obj.ptr, flag)

Note that obj or obj.ptr is never used by the backend
and the obj argument is only used to derive the type.
This code sequence is subject to potential llvm CSE when

  • obj is the same .e.g., nullptr
  • flag is the same
  • metadata type is different, e.g., typedef of struct "s" and strust "s".

In the above, we don't want CSE since their metadata is different.

This patch change IR builtin to

llvm.bpf.btf.type.id(seq_num, flag)  !type

and seq_num is always increasing. This will prevent potential
llvm CSE.

Also report an error if the type name is empty for
remote relocation since remote relocation needs non-empty
type name to do relocation against vmlinux.

Diff Detail

Event Timeline

yonghong-song created this revision.Aug 3 2020, 5:17 PM
yonghong-song requested review of this revision.Aug 3 2020, 5:17 PM
ast added a comment.Aug 3 2020, 5:47 PM

Is it a cleanup or is it a fix for some bug? If latter there should be a new test for it?

anakryiko accepted this revision.Aug 3 2020, 5:52 PM

Tested locally. Previously failing tests are now passing. All recorded relocations look correct. Thanks!

This revision is now accepted and ready to land.Aug 3 2020, 5:52 PM
In D85174#2192269, @ast wrote:

Is it a cleanup or is it a fix for some bug? If latter there should be a new test for it?

This is also to fix the CSE issue where two builtins are CSE'ed if one has type "struct t" and the other is "t" where "t" is defined as "typedef struct t __t". I will add a clang test to capture the generated IR.

add a test case in clang where testing type existence of a named struct and the typedef of that named struct will result in different IR intrinsic func arguments which will prevent CSE which may lose one of ditypes.

This revision was landed with ongoing or failed builds.Aug 4 2020, 4:30 PM
This revision was automatically updated to reflect the committed changes.