I can rename these things, but tbh I don't think this functionality would be useful anywhere outside BPF, thus such renaming would be kind-of deceptive (and in case it would be useful, the renaming could be done at the time of second use).

I will try to review other parts of code in the next few days.

In D156497#4621699, @tamird wrote:

@yonghong-song what do you think of that suggestion?

Thanks. This is indeed much better to detect during insn matching stage.

Thanks @eddyz87 for a detailed example. The following is my understanding.

• In any case, there is a compilation error. The binary should not be used since it is invalid with large chance.
• The continual optimization will be done with either existing code or this patch (Constant 0 -> UNDEF)
• For existing code, for *extra* parameters, the compilation will proceed with those extra parameters as constant 0.
• With this patch, for *extra* parameters, the compilation will proceed with those extra parameters as undefined.
• 'constant 0' may simplify the code and largely preserve the code flow.
• 'undefined' may also simplify the code as compiler may discard some codes involved with 'undefined' values.

Thanks, Eduard. Your change makes sense. I wish I knew isZExtFree() for load insn much earlier so we do not need to have pass to remove redundant codes....

Ah, right right - sorry, failed to push. Here it is: 2993243c45abdb4f2bc3979336d054be165b1134

In D156726#4593671, @dblaikie wrote:

Fixed in 19c216b5ab7c34c297b4c3c4da64b49b3c830a3a

We must be misunderstanding each other. This patch doesn't change the error messages presented to the user at compilation time.

The purpose of the tests in this patch are to show how it is trivially easy to incorrectly use the LLVM C API such that diagnostics "errors" do not halt compilation.

The purpose of the non-test changes in this patch are to cause the resulting program to fail to verify. Before this patch the resulting program would appear to be valid, but violate the user's intent at runtime.

In D156497#4583451, @tamird wrote:

With current clang, could you show me the error message with 'constant 0' vs. 'undef'? We can then decide to pick the one which is more user friendly.

You're referring to verifier errors, right? With "constant 0" there is no verifier error - the program will just do the wrong thing (not what the programmer intended). I don't have the "undef" verifier error on hand, I can get that to you tomorrow.

Still looks good. Thanks!

Okay, I see. The key is Multiple virtual register defs in SSA form and the resulted IR is broken w.r.t. SSA.
Ya, I mentioned earlier, indeed your change is good and new IR is much more intuitive from data flow perspective.

In D157805#4583326, @eddyz87 wrote:

In D157805#4583287, @yonghong-song wrote:

LGTM. Thanks for detailed comments to show why generated code is incorrect. Do you think we should backport this fix to llvm17?

On the one hand the code dates back to Dec 2019 and I don't see it affecting code generation in practice (at-least for our selftests). On the other hand it is really easy to backport. I'd say we should backport it.

Your approach seems fine and probably better esp. for CORE for stores. Another solution is similar to D157805 where we can clear 'killed' for the 'out' parameter, right?

With LLVM_ENABLE_EXPENSIVE_CHECKS, even with this patch, there are still some selftest llvm crashes. I guess you are probably working on this (e.g. D157806).

LGTM. Thanks for detailed comments to show why generated code is incorrect. Do you think we should backport this fix to llvm17?

In D156497#4578070, @tamird wrote:

Yes, thank you. As mentioned earlier, we have fixed this bug. The purpose of this patch is to be defensive and avoid this footgun for future users of the LLVM C API.

Do you feel that this patch degrades the quality of the implementation?

Based on the discussion with @eddyz87, the following llvm diagnose implementation shows why clang will through an error rather than letting compilation continue:

void LLVMContext::diagnose(const DiagnosticInfo &DI) {
  if (auto *OptDiagBase = dyn_cast<DiagnosticInfoOptimizationBase>(&DI))
    if (LLVMRemarkStreamer *RS = getLLVMRemarkStreamer())
      RS->emit(*OptDiagBase);

LGTM except a few nits!

So if I understand correctly, BPF backend can handle clang frontend generated IR properly and issue proper warning. But with rust frontend, the IR is a little bit different and BPF backend will behave differently and allow to generate illegal code. Is this right? The bpf-unsupported-emit.c is exactly the case to demonstrate that rust frontend may generate different IR and the BPF backend will generate incorrect code. Is my understanding correct?

Okay, the new error message aggregate returns are not supported looks good to me. thanks!

The error message is not what I envisioned.

$ cat t.c
struct t {
   int a;
};
struct t foo(int a, int b) {
  struct t tmp;
  tmp.a = a + b;
  return tmp;
}
$ clang --target=bpf -O2 -g -c t.c
t.c:4:10: error: functions returning through a pointer parameter are not supported
    4 | struct t foo(int a, int b) {
      |          ^
1 error generated.
$

From source code perspective, there is no 'returning through a pointer parameter'.
The source code shows 'returning a struct'.
The error message can be 'functions returning a struct are not supported'.

LGTM except one minor suggestion.

I'll abandon this, but it may be worthwhile to implement BPF-implies-fno-builtin *in clang*, if possible.

I checked the code and found that ExternalSymbolSDNode is indeed corresponding to builtin/library functions.
In SelectionDAG/SelectionDAG.cpp, we have

                    getExternalSymbol(TLI->getLibcallName(RTLIB::MEMCPY),
...
                    getExternalSymbol(TLI->getLibcallName(LibraryCall),
...
                    getExternalSymbol(TLI->getLibcallName(RTLIB::MEMMOVE),
...
                    getExternalSymbol(TLI->getLibcallName(LibraryCall),
...
        getExternalSymbol(BzeroName, TLI->getPointerTy(DL)), std::move(Args));
...
                     getExternalSymbol(TLI->getLibcallName(RTLIB::MEMSET),
...
                    getExternalSymbol(TLI->getLibcallName(LibraryCall),
...
  SDValue Callee = getExternalSymbol(TLI->getLibcallName(LC),

I applied this patch and did an experiment with bpftool linker functionality. The following are details.

• Add more tests in assembler-disassembler-v4.s and gotol.ll.

Three major changes in this patch:

• for ldsx insns, remove 32bit ldsx insns (1-byte and 2-byte sign extension) since the ldsx insn expects to sign extension all the way up to 8-byte and normal 32bit insn (e.g. BPF_ALU) expects to zero out the top bits. Instead do a ldbsx/ldhsx and then take the lower 4 byte to extract 32bit value. This also resolved one disasm issue reported by Eduard.
• for movsx insn, for 32bit sign extenstion to 64bit. Match both "sext_inreg GPR:$src, i32" (left and right shifting) and "sext GPR32:$src".
• Add an internal flag to control when to generate gotol insns in BPFMIPeephole.cpp. This permits a simpler test for gotol insns.

Could you please also add a few tests for gotol?

In D144829#4519036, @eddyz87 wrote:

I tried adding a test similar to assemble-disassemble.ll:

// RUN: llvm-mc -triple bpfel --mcpu=v4 --assemble --filetype=obj %s \
// RUN:   | llvm-objdump -d --mattr=+alu32 - \
// RUN:   | FileCheck %s

// CHECK: d7 01 00 00 10 00 00 00	r1 = bswap16 r1
// CHECK: d7 02 00 00 20 00 00 00	r2 = bswap32 r2
// CHECK: d7 03 00 00 40 00 00 00	r3 = bswap64 r3
r1 = bswap16 r1
r2 = bswap32 r2
r3 = bswap64 r3

// CHECK: 91 41 00 00 00 00 00 00	r1 = *(s8 *)(r4 + 0x0)
// CHECK: 89 52 04 00 00 00 00 00	r2 = *(s16 *)(r5 + 0x4)
// CHECK: 81 63 08 00 00 00 00 00	r3 = *(s32 *)(r6 + 0x8)
r1 = *(s8 *)(r4 + 0)
r2 = *(s16 *)(r5 + 4)
r3 = *(s32 *)(r6 + 8)

// CHECK: 91 41 00 00 00 00 00 00	w1 = *(s8 *)(r4 + 0x0)
// CHECK: 89 52 04 00 00 00 00 00	w2 = *(s16 *)(r5 + 0x4)
w1 = *(s8 *)(r4 + 0)
w2 = *(s16 *)(r5 + 4)

// CHECK: bf 41 08 00 00 00 00 00	r1 = (s8)r4
// CHECK: bf 52 10 00 00 00 00 00	r2 = (s16)r5
// CHECK: bf 63 20 00 00 00 00 00	r3 = (s32)w6
r1 = (s8)r4
r2 = (s16)r5
r3 = (s32)w6
// Should this work as well: r3 = (s32)r6 ?

// CHECK: bc 31 08 00 00 00 00 00	w1 = (s8)w3
// CHECK: bc 42 10 00 00 00 00 00	w2 = (s16)w4
w1 = (s8)w3
w2 = (s16)w4

// CHECK: 3f 31 01 00 00 00 00 00	r1 s/= r3
// CHECK: 9f 42 01 00 00 00 00 00	r2 s%= r4
r1 s/= r3
r2 s%= r4

// CHECK: 3c 31 01 00 00 00 00 00	w1 s/= w3
// CHECK: 9c 42 01 00 00 00 00 00	w2 s%= w4
w1 s/= w3
w2 s%= w4

And it looks like some instructions are not printed correctly:

$ llvm-mc -triple bpfel --mcpu=v4 --assemble --filetype=obj /home/eddy/work/llvm-project/llvm/test/CodeGen/BPF/assembler-disassembler-v4.s | llvm-objdump -d --mattr=+alu32 -

<stdin>:	file format elf64-bpf

Disassembly of section .text:

0000000000000000 <.text>:
       0:	d7 01 00 00 10 00 00 00	r1 = bswap16 r1
       1:	d7 02 00 00 20 00 00 00	r2 = bswap32 r2
       2:	d7 03 00 00 40 00 00 00	r3 = bswap64 r3
       3:	91 41 00 00 00 00 00 00	w1 = *(s8 *)(r4 + 0x0)
       4:	89 52 04 00 00 00 00 00	w2 = *(s16 *)(r5 + 0x4)
       5:	81 63 08 00 00 00 00 00	<unknown>
       6:	91 41 00 00 00 00 00 00	w1 = *(s8 *)(r4 + 0x0)
       7:	89 52 04 00 00 00 00 00	w2 = *(s16 *)(r5 + 0x4)
       8:	bf 41 08 00 00 00 00 00	r1 = (s8)r4
       9:	bf 52 10 00 00 00 00 00	r2 = (s16)r5
      10:	bf 63 20 00 00 00 00 00	r3 = (s32)w6
      11:	bc 31 08 00 00 00 00 00	w1 = (s8)w3
      12:	bc 42 10 00 00 00 00 00	w2 = (s16)w4
      13:	3f 31 01 00 00 00 00 00	r1 s/= r3
      14:	9f 42 01 00 00 00 00 00	r2 s%= r4
      15:	3c 31 01 00 00 00 00 00	w1 s/= w3
      16:	9c 42 01 00 00 00 00 00	w2 s%= w4

I'm not sure if this is an issue with disassembler or some additional --mattr options are needed.

• remove a copy-paste comment from s390 arch

• Dropping 'CPUv4' in some variable/function names and also in debug flags.

• rename some insn names or mode names (movs -> movsx, lds -> ldsx, MEMS -> MEMSX) etc to be consistent with kernel.
• add 5 llc flags to control on/off for each kind of insn (sdiv/smod, ldsx, movsx, bswap, gotol) to debugging purpose.

I see. thanks for explanation!

sounds good to me. could you summarize what caused the memory leak and what is the fix?

In D147990#4465164, @eddyz87 wrote:

Hi Yonghong,

Could you please take a look?
I checked this revision against kernel master and see code generation change in a single test: get_branch_snapshot.c, the test contains pattern in question. No veristat changes for Cilium programs [2].
I also verified that selftests at revision [1] (the last revision before your fixes for check_cond_jmp_op()) are passing when using this revision (and loop6 fails if main is used instead of this revision).
Unfortunately, after thinking on loop6 behavior a bit more I can't suggest a better solution than this.

[1] 360cd42c4e95 ("io_uring: optimise io_req_local_work_add")

In D144829#4465193, @eddyz87 wrote:

Hi Yonghong,

What is the current plan for these changes?
I'd like to refresh D140804 to make BPF_ST instruction available for cpu v4.
I see that the latest CI run failed because of libcxx issue, I think it is completely unrelated to this revision, see here.

I am okay with this change. Once you used this patch and implemented the mechanism inside Google. Could you send a follow-up summary on what the implementation looks like in Google for the thread:

https://lore.kernel.org/bpf/CAKH8qBt4xqBUpXefqPk5AyU1Rr0-h-vCJzS_0Bu-987gL4wi4A@mail.gmail.com/

• avoid changing conditions during JMP -> JMPL conversion. Otherwise, verification may fail in some cases.

• added support of new instructions in inline assembly.

LGTM. Thanks!

Sounds good to me. This way, functions and stmts with nomerge attribute should work for bpf backend.

Add @nickdesaulniers as the reviewer as well. @nickdesaulniers could you help take a look at the patch as well?

@nikic Thanks for your comment and pointer! I will study Webassembly/AMDGPU/NVPTX to see how they resolve their respective 'legality' issues and will report back once I got some understanding about their strategy and commonality/difference w.r.t. BPF.

@nikic Could you comment on @davidxl suggestion? I would be great if we can find a path forward for this particular issue. Thanks!

• Fixed previous llvm-objdump issue for '> 16bit' 'gotol' insns.
• Now basic functionality for cpu=v4 should be complete for llvm, further work will focus on kernel.

• use one hook TTI->needsPreserveRangeInfoInVerification() instead of three hooks
• remove unused checks in the tests

• Fixed issues reported by Eduard
• llvm-objdump issue (as stated in 'Summary') is not resolved yet.

@davidxl Thanks for your suggestions. Adding a single TTI->needsPreserveRangeInfoInVerification() sounds a reasonable idea. Legalization of IR for BPF verification is a great idea but it requires more thought, e.g., how IR will be enhanced to encode verification requirement and how middle end optimization reacts to such requirement as typically it is not clear whether a particular optimization will hurt bpf verification or not unless running though bpf verifier or having a deep knowledge of bpf verifier. We can continue to think and discuss 'legalization of IR for BPF verification' idea, but it could be great we can have current TTI->needsPreserveRangeInfoInVerification() approach which gcc community intends to do the same.

@dblaikie could you help take a look at this patch? Similar to https://reviews.llvm.org/D143966, this patch is the clang frontend change to support new btf_type_tag format, as we discussed and agreed with gcc community (https://lore.kernel.org/bpf/87r0w9jjoq.fsf@oracle.com/). Please let us know if you have any questions. Thanks!

@dblaikie Could you help take a look at this patch? Previously we have implemented btf_type_tag support in clang. After some discussion with gcc folks (https://lore.kernel.org/bpf/87r0w9jjoq.fsf@oracle.com/), we decided to change the dwarf format a little bit to make btf_type_tag more extensible. Currently, dwarf is used by pahole which supports the old format. @eddyz87 has implemented pahole change to support new format. So for the
time being, pahole will support both old and new format. And llvm dwarf output should also support both old and new format for a while so old pahole and new pahole should both work. Once minimum llvm requirement is lifted in linux kernel, we can remove support for old format.

In D150017#4326665, @aaron.ballman wrote:

LGTM, but please also add a release note about the fix.

Sorry, not sure I understand. The only way to preserve old interface for DILineInfo that I see is to collect all lines stored in .BTF.ext, sort those lines by file name, line number and create std::string instances with "fake" source code for each file (which is a bit wasteful from cpu/memory pov). Is that what you mean or am I missing something?

From BPF perspective, it looks good to me. The only thing I am not sure is in struct DILineInfo, std::optional<StringRef> LineSource; is added which sounds a little bit weird since 'Source' is there. But the 'LineSource' is used to differentiate from dwarf 'Source'. If this is not preferred, we could remove it and hide the LineSource info in BTFContext instead.

I see the following in the Summary:

Type tag for CVR modifier type

Related to btf_type_is_modifier() issue, Not that depending on call site, sometimes typedef is skipped and sometimes are not. So we could keep btf_type_is_modifier() as is and modify the call size to not skip typedef in specific places. Or we could remove typedef from the function and add a 'or' in call side if typedef needs to be skipped. The following are a few examples in kernel/bpf/btf.c:

Ping again. @mkazantsev @nikic @chandlerc @lebedev.ri @spatel Could you help review the patch and share your opinion? Thanks!

@mkazantsev @nikic @chandlerc @lebedev.ri @spatel Ping. Could you help take a look at this patch? Thanks!

@nikic @mkazantsev I created another diff https://reviews.llvm.org/D147968 which use a different approach (through TTI) and more description on why we want to use TTI hooks. Could you take a look there as well? Thanks!

• remove function 'annotation' like dso_local, fastcc, local_unnamed_addr, unnamed_addr, as they won't affect argspromotion transformation.

what about combining arguments in the backend (basically undoing this transformation)? that would generalize better to other passes and input IR

• for added test, do not use -O2, use -passes=argpromotion instead for opt. Also put the test under test/Transformations/ArgumentPromotion/BPF directory similar to ArgumentPromotion/X86.

• add a test case

@nikic This is a BPF limitation. See the following kernel document:

https://www.kernel.org/doc/html/latest/bpf/bpf_design_QA.html#q-can-more-than-5-function-arguments-be-supported-in-the-future

Hi, @aeubanks , this patch is hurting bpf backend.
The bpf backend calling convention is only allowing 5
parameters in registers and does not allow pass arguments
through stacks. But ArgumentPromotionPass could increase
the number of arguments for the function. If the eventual
number of arguments is more than 5, bpf backend will
have a fatal error.

• add TTI hook for LICM/HoistMinMax transformations.
• add detailed code analysis in commit message.

Overall looks good. Just a few minor comments above.

• do not add TTI to InstCombinerImpl, use the existing one in InstCombiner
• add a code example to illustrate the problem in Summary

Do we have cases where name_off is not 0? If that is the case, should we just go ahead print the name? If not, maybe we should not print name_off = 0 at all?

• Using TTI hooks instead of flags

LGTM. Thanks!

The previous patch caused several bpf selftest failures. Remove two flags and changed a few tests so bpf selftests can pass.

In D147707#4255094, @niclashedam wrote:

Thanks for the review. I have a colleague at the IT University of Copenhagen doing a PhD on program verification for computational storage. However, for now, it's an open question.
We do some runtime checks, but the real issue is that when compiling BPF for computational storage, the device characteristics (stack size, helper functions, etc.) are unknown. As such, the verification must happen on the device, and the question is how we efficiently do that and how we convey potential verification problems back to the host.

LGTM. How do you verify bpf program in your environment?