User Details
- User Since
- Oct 4 2018, 11:50 PM (259 w, 1 d)
Tue, Aug 29
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).
Mon, Aug 28
I will try to review other parts of code in the next few days.
Sun, Aug 27
Aug 23 2023
Aug 21 2023
Thanks. This is indeed much better to detect during insn matching stage.
Aug 18 2023
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....
Aug 16 2023
Ah, right right - sorry, failed to push. Here it is: 2993243c45abdb4f2bc3979336d054be165b1134
Aug 14 2023
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.
Aug 13 2023
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.
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?
Aug 12 2023
Aug 10 2023
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);
Aug 4 2023
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?
Aug 3 2023
Jul 31 2023
Okay, the new error message aggregate returns are not supported looks good to me. thanks!
Jul 30 2023
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'.
Jul 29 2023
LGTM except one minor suggestion.
Jul 28 2023
I'll abandon this, but it may be worthwhile to implement BPF-implies-fno-builtin *in clang*, if possible.
Jul 26 2023
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.
Jul 25 2023
- Add more tests in assembler-disassembler-v4.s and gotol.ll.
Jul 24 2023
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?
Jul 19 2023
- remove a copy-paste comment from s390 arch
Jul 17 2023
- Dropping 'CPUv4' in some variable/function names and also in debug flags.
Jul 14 2023
Jul 12 2023
- 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.
Jul 7 2023
I see. thanks for explanation!
Jul 6 2023
sounds good to me. could you summarize what caused the memory leak and what is the fix?
Jul 2 2023
Jun 29 2023
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/
Jun 26 2023
- avoid changing conditions during JMP -> JMPL conversion. Otherwise, verification may fail in some cases.
Jun 25 2023
- added support of new instructions in inline assembly.
Jun 23 2023
LGTM. Thanks!
Jun 18 2023
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?
Jun 14 2023
@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.
Jun 7 2023
May 18 2023
- 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.
May 17 2023
- 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.
May 15 2023
@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.
May 8 2023
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?
May 7 2023
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.
May 5 2023
May 2 2023
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:
May 1 2023
Ping again. @mkazantsev @nikic @chandlerc @lebedev.ri @spatel Could you help review the patch and share your opinion? Thanks!
Apr 20 2023
@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!
Apr 19 2023
- remove function 'annotation' like dso_local, fastcc, local_unnamed_addr, unnamed_addr, as they won't affect argspromotion transformation.
Apr 17 2023
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
Apr 16 2023
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.
Apr 14 2023
- 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
Apr 13 2023
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
Apr 12 2023
LGTM. Thanks!
The previous patch caused several bpf selftest failures. Remove two flags and changed a few tests so bpf selftests can pass.
Apr 10 2023
Apr 9 2023
LGTM. How do you verify bpf program in your environment?