This is an archive of the discontinued LLVM Phabricator instance.

[BPF] Use elementtype attribute for preserve.array/struct.index intrinsics
ClosedPublic

Authored by nikic on Jul 16 2021, 1:18 PM.

Details

Reviewers
yonghong-song
aeubanks
Group Reviewers
Restricted Project
Summary

Use the elementtype attribute introduced in D105407 for the llvm.preserve.array/struct.index intrinsics. It carries the element type of the GEP these intrinsics effectively encode.

This patch:

  • Adds a verifier check that the attribute is required.
  • Adds it in the IRBuilder methods for these intrinsics.
  • Autoupgrades old bitcode without the attribute.
  • Updates the lowering code to use the attribute rather than the pointer element type.
  • Updates lots of tests to specify the attribute.
  • Adds -force-opaque-pointers to the intrinsic-array.ll test to demonstrate they work now.

Diff Detail

Event Timeline

nikic created this revision.Jul 16 2021, 1:18 PM
nikic requested review of this revision.Jul 16 2021, 1:18 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 16 2021, 1:18 PM
nikic added inline comments.Jul 16 2021, 1:23 PM
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
303

I noticed too late that this intrinsic also uses the element type to get the alignment. I'll have to follow up with the same change for this intrinsic.

Though I'm not completely sure if this is actually used, because no tests fail if I replace this with dummy values like 1 or 128.

aeubanks accepted this revision.Jul 16 2021, 3:13 PM
aeubanks added a subscriber: aeubanks.

awesome!

This revision is now accepted and ready to land.Jul 16 2021, 3:13 PM
nikic updated this revision to Diff 359543.Jul 17 2021, 1:07 AM

Add CHECK-LABEL to autoupgrade test. It's failing on pre-merge checking and the output is not helpful...

nikic closed this revision.Jul 17 2021, 2:23 AM

Landed as https://github.com/llvm/llvm-project/commit/be5af50e7d028849bf2fab5f4b0f2ad36ae56e11, missed the "Differential Revision" tag.

For the record, the problem with the autoupgrade test was PEBKAC: The textual diff that gets uploaded to Phabricator didn't include the contents of the binary bitcode file, and apparently LLVM happily interprets an empty bitcode file as an empty module.

I just tested the patch with kernel bpf selftests and it works fine. The patch looks good to me too.

llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
303

This is for llvm.preserve.union.access.index. It is used only if the union member type is a bitfield and we don't have any usecase and test case for that. That is why we didn't hit it.

It is okay for now. I probably will change to use getBaseElementType(Call) later by myself.

nikic added inline comments.Mar 7 2022, 8:14 AM
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
303

I want to check back here on what we should do with this intrinsic. Should it be changed to use elementtype as well?

A possible alternative is to only add an align attribute to it, as it's the only part being actually used.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 8:14 AM