@rsmith @eli.friedman Ping again. Do you have any comments on my proposed clang/IR intrinsics? Thanks!

remove bpf offsetreloc option and use FileCheck in the test.

remove BPF "checkoffsetreloc" subtarget option as just honoring what user says. Later on, if needed, we can introduce a pass to help show user what is transformed and what is not so user can decide whether there is any missing __builtin_preserve_access_index()'s.

@rsmith @eli.friedman Do you have any comments on the clang intrinsic interface in this patch and the llvm intrinsics interface at https://reviews.llvm.org/D61810?

@eli.friedman @rsmith @lebedev.ri I just loaded a new set of patches. Notable changes include

• introduce clang intrinsic base = __builtin_preserve_access_index(base), which is used to identify relocatable geps. This is a buy-in approach from user so we do not pay performance penalty for transforming gep's to intrinsics and later back to gep with some performance loss.
• for IR intrinsics preserve_*_access_index, struct/union names are removed from the argument list. Instead a new metadata type MD_preserve_access_index is introduced. This metadata has struct/union debuginfo type and attached to preserve_*_access_index.

removed typename from intrinsics, using metadata (ditype) instead

add clang builtin __builtin_preserve_access_index

I did some analysis on the performance side, specifically, # of instructions generated.
My example is cilium at https://github.com/cilium/cilium/tree/master/bpf, bpf program bpf_lxc.c.

This update of three patches mostly a few intrinsic format changes (array +num_of_zeros for gep, array/struct -inst_name), and correspond adjustments to fix bugs.
This will unblock other kernel bpf developers which can work in parallel in kernel side.

changes due to intrinsics change

change new intrinsic format, add num_of_zeros for array, remove inst_name

array index may not be constant and remove inst_name

@eli.friedman thanks for the review. I just did some experiments with some performance sensitive bpf programs. It looks the approach I am taking here cause worse case 30% instructions generated by llvm at -O2 level.
So the current approach to convert to intrinsics blindly and later on convert back unnecessary intrinsics to regular GEP and necessary intrinsics to relocatable GEP won't work.
The main reason for performance degradation is:

• intrinsics causing less optimal codes which may contributes 5% performance loss for a relative small routines, for a large routines, I have see the loss is more than 20%.
• intrinsics impact inline decision and less inlining has negative impacts on the performance.

@rsmith @eli.friedman Hi, I just implemented a new version which generates intrinsics only. Three intrinsics are implemented for array/union/structure separately as their handling and parameters are different. Could you take a look? Thanks!

@lebedev.ri For comments "What about other targets, does this need some default expansion?", currently these intrinsics only used by bpf and lowered to GEP in an IR pass implemented by bpf backend. I can certainly implement some default expansion if it is desirable. Is the SelectionDAG the proper place to implement the default lowering?

use the new preserve_*_access_index intrinsics

split intrinsics into three, and add langdef.

do not generate gep any more, only intrinsics

@rsmith @eli.friedman Thanks for your comments. I fully agree that it seems awkward that we have both GEP and intrinsic generation. I will try to do some experiments here to only have intrinsic generation. My only concern is possible performance degradation. I will post here once I got concrete implementation and numbers.

getAsInstruction probably doesn't do what you want here. It actually creates a new Instruction*, instead of casting the expression. Probably this is the source of the verifier error.

@eli.friedman I just uploaded a new revision which uses a newly-added intrinsics to pass information from clang generated IR to BPF IR pass. Could you take a look and give your opinion on this approach?
This patch is not mergeable as I have a llvm verification failure described in my previous comments. If you have some insight on this issue, please let me know. Thanks!

My current implementation has an issue like below:

A new apporach to use intrindics to pass info. from clang is used. The following two patches implements the intrinsics:

https://reviews.llvm.org/D61809
https://reviews.llvm.org/D61810

@lebedev.ri Thanks for looking at this patch. The following two other patches are correlated so I just added them here.

https://reviews.llvm.org/D61809
https://reviews.llvm.org/D61524

Could you be more specific where to add langref for this intrinsic?

@lebedev.ri Thanks for the comment. This patch is not ready to land yet. Yes, tests are missing and I am going to add tests later.
More importantly, I want to get a sense whether what I am implementing here is the right direction or not.
The following two other patches are also related to describe our end goal:

https://reviews.llvm.org/D61810
https://reviews.llvm.org/D61524

clang encodes struct/union typename, field_index, array subscript and the original getelementptr base address into preserve_di_access_index intrinsics. this way, the BPF IR passes can just look at these intrinsics to get the original type info in order to construct the complete gep access pattern.

@efriedma thanks for the comments. I will take a look at clang gep generation to see whether relocatable gep can be done where.

merged. https://reviews.llvm.org/rL359904

Thanks for the fix! Please go ahead to land by yourself if you have write permission. Otherwise, let me know I can land for you.

remove __bpf.

This definitely more llvm-like. Thanks for the change!

I read through the link and yes the code looks okay. I think this is a communication issue.
It would be good next time if you have better commit messages which provides enough info. about the change so reviewers can at least
take a look knowing what happens. In the case the reviewer requested a change, it would be good to send out information (as you did)
and wait for acknowledgement before merging.

Abandon this patch as the proposed fix is not correct. I have put relative information in the bug https://bugs.llvm.org/show_bug.cgi?id=40170.

Yes, I do. I can follow the instructions at https://llvm.org/docs/Phabricator.html to commit this change. Thanks!

@dblaikie As I mentioned in one of my earlier comments, after some analysis, I think this patch definitely not the right way to fix the problem. We just cannot default to the main file if the DIFile source is not available.

@lhames Pinging. Could you take a look whether the modified patch addressed your comments or not? Thanks!

@lhames Just ping, did you get a chance looking at the updated patch? Thanks!

Hi, @lhames, could you check whether my change addressed your concern or not? Thanks!

change one comment from C style to C++ style

@lhames Thanks for the review. I just addressed your comments to emit all remaining not-yet-emitted sections. Could you take a look?
Also since the approach for this patch is preferred, I have abandoned my another patch D56729. Thanks!

Abandon this patch. D55943 is the recommended approach.

address Lang's comments. Emit all not-yet-loaded sections.

Hi, @lhames For this patch, if ProcessAllSections is set to true, all sections will be loaded.
I am not sure whether this is the original intention of not as it says ProcessAllSections, not meaning LoadAllSections.

Hi, @lhames this is an alternative implementation w.r.t. D55943. In D55943, if ProcessAllSections is set to true, all sections will be loaded.
I am not sure whether this is the original intention of not as it says ProcessAllSections, not meaning LoadAllSections.

@scott.linder Thanks! It would be good if you could give at least an initial, even rough, analysis.

@dblaikie @scott.linder Have you got time to look at this issue? Looks like a trivial test case will be able to reproduce the problem. I have the simple example in the previous comments. Practically, -gdwarf-5 -gembed-source is broken now.

Hi, @lhames Pinging, have you got time to take a look at this issue?

I found an easy way to reproduce the issue and also did some preliminary analysis. I have filed a bug https://bugs.llvm.org/show_bug.cgi?id=40170. The reproducible case and my analysis is below:

@labath No problem. Yes, I just searched and find you also contributed patch(es) there in non architecture-dependent way. @lhames is in the review list. Thanks!

I uploaded a test case to demonstrate the use case related to BPF and .BTF section, in case that you want to reproduce in this specific use case context.

The alternative method (put most of implementation under lib/Target/BPF with slight refactoring at the core) is used. Abandon this one.

@echristo @aprantl @dblaikie
This patch and two other patches (D55755, D55752) implemented a different mechanism
to generate BTF than patch D53736. D53736 has its most implementation under
lib/CodeGen/AsmPrinter. The new mechanism put most implementation under BTF target.

@echristo @aprantl @dblaikie
This patch and two other patches (D55756, D55752) implemented a different mechanism
to generate BTF than patch D53736. D53736 has its most implementation under
lib/CodeGen/AsmPrinter. The new mechanism put most implementation under BTF target.

(1) https://reviews.llvm.org/D55755, moving header files
(2) https://reviews.llvm.org/D55756, changes some fields from private to protected
(3) https://reviews.llvm.org/D55752, changes only under target BPF

It will be good if you can help review all related patches and provide your opinions soon.
Thanks! Real appreciated.

@echristo @aprantl @dblaikie This patch has stuck in this state for more than one month and did not make progress.
I have suggested a possible way to move some header files to include directory and then we will be
able to move BTFDebug.{cpp,h} under BPF directory. I have implemented this approach with the
following three patches for easy to see pro and cons of two approaches:

(1) https://reviews.llvm.org/D55755, moving header files
(2) https://reviews.llvm.org/D55756, changes some fields from private to protected
(3) https://reviews.llvm.org/D55752, changes only under target BPF

refactor the previous change to separate Debug info changes into
https://reviews.llvm.org/D55755 (for moving related header files into include directory)
and
https://reviews.llvm.org/D55756 (for exposing some private fields as protected)

In one of my earlier comments, I mentioned it is possible to push the implementation into BPF target directory with the need
moving three private headers to include/llvm/CodeGen directory.
I did not get opinion whether that implementation is preferred or not, but I feel that maybe it is a good idea just to go ahead
with a diff which may facilitate discussion and move review/discussion faster.
I created another diff, https://reviews.llvm.org/D55752, which did this implementation.