[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

ast accepted this revision. ast added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + eddyz87 wrote: > ast wrote: > > eddyz87 wrote: > > > ast

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

ast added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + eddyz87 wrote: > ast wrote: > > If I'm reading this correctly wrapping with

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

ast added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + If I'm reading this correctly wrapping with preserve_static_offset doesn't prevent

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

ast added a comment. In D133361#4637008 , @eddyz87 wrote: > In D133361#4629292 , @ast wrote: > >> ... >> Right. Such recursive propagation of PAI is necessary. For btf_tag we cannot >> do it. Always propagating

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

ast added a comment. In D133361#4629081 , @eddyz87 wrote: > Note on current propagation logic: whenever there is an expression E of type > T, where T is a struct annotated with btf_decl_tag("ctx"), all usages of E > are traversed recursively visiting

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

ast added a comment. > 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 agree that it's not

[PATCH] D144829: [BPF] Add a few new insns under cpu=v4

ast accepted this revision. ast added a comment. lgtm. @eddyz87 pls take a look Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144829/new/ https://reviews.llvm.org/D144829 ___ cfe-commits mailing list

[PATCH] D144829: [WIP][BPF] Add a few new insns under cpu=v4

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. overall looks good. one small nit. Comment at: llvm/lib/Target/BPF/MCTargetDesc/BPFMCFixups.h:17 +enum FixupKind { + // These correspond directly to R_390_* relocations. +

[PATCH] D144829: [WIP][BPF] Add a few new insns under cpu=v4

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:56 def BPFNoALU32 : Predicate<"!Subtarget->getHasAlu32()">; +def BPFHasCPUv4_ldsx : Predicate<"Subtarget->getCPUv4_ldsx()">; +def BPFHasCPUv4_movsx : Predicate<"Subtarget->getCPUv4_movsx()">;

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

ast added a comment. In D142046#4066758 , @yonghong-song wrote: > @ast With this patch, gentoo clang compilation will hit the warning even if > people appends -fno-stack-protector. Is this okay? In general, if the option > is '-fstack-protector

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. This approach looks better to me than NVPTX warn and I agree with Ed that it's better to leave NVPTX as-is to avoid any regression. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D132144: [Clang][BPF] Support record argument with direct values

ast added a comment. clang -target bpf -O2 -S t3.c t3.c:1:5: error: defined with too many args this will be the case when backend needs to emit the code, but if the function is always_inline it can have more than 5 arguments, right? Would be good to have a test for that to make sure when

[PATCH] D103549: [POC] Put annotation strings into debuginfo.

ast added a comment. > Yep LLVM does have some custom attributes and such, eg: > https://github.com/llvm/llvm-project/blob/effb87dfa810a28e763f914fe3e6e984782cc846/llvm/include/llvm/BinaryFormat/Dwarf.def#L592 This is great insight! That should work. We need to make sure that

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. Makes sense to me. Only BPF target will notice this difference. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100567/new/

[PATCH] D93103: Enable the _ExtInt extension on the BPF Target

ast requested changes to this revision. ast added a comment. This revision now requires changes to proceed. What's a use case? The test is necessary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93103/new/ https://reviews.llvm.org/D93103

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFMIChecking.cpp:199 + unsigned newOpcode; + switch(MI.getOpcode()) { + case BPF::XFADDW32: newOpcode = BPF::XADDW32; break; yonghong-song wrote: > yonghong-song wrote: > > ast wrote: > >

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFMIChecking.cpp:199 + unsigned newOpcode; + switch(MI.getOpcode()) { + case BPF::XFADDW32: newOpcode = BPF::XADDW32; break; With this logic in place Andrii has a point. There is no need

[PATCH] D72184: [BPF] support atomic instructions

ast added a comment. Looks like the test didn't change. Only commit log... that's fine. I think the diff is ready to land, but let's wait for the kernel patches to be ready as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72184/new/

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:14 +; int test_xchg_64(long *p, long v) { +; return __sync_lock_test_and_set(p, v); +; } yonghong-song wrote: > ast wrote: > > test_and_set is not the same as xchg. > > xchg

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:14 +; int test_xchg_64(long *p, long v) { +; return __sync_lock_test_and_set(p, v); +; } test_and_set is not the same as xchg. xchg doesn't do comparison. Repository: rG LLVM

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:783 + let Inst{47-32} = addr{15-0}; // offset + let Inst{11-8} = new; + let Inst{7-4} = BPF_CMPXCHG.Value; yonghong-song wrote: > jackmanb wrote: > > If we go down the route of

[PATCH] D72184: [BPF] support atomic instructions

ast added a comment. looks good. Before landing we need to agree on the full set of instructions that -mcpu=v4 will support. atomic_fetch_or|xor|and are probably needed as instructions. The kernel JIT will generate x86 cmpxchg for them. Because if llvm generates bpf cmpxchg insn then we'd need

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:684 + let Inst{47-32} = addr{15-0}; // offset + let Inst{11-8} = val; + let Inst{7-4} = Opc.Value; jackmanb wrote: > yonghong-song wrote: > > jackmanb wrote: > > > jackmanb wrote: > >

[PATCH] D91489: BPF: make __builtin_btf_type_id() return 64bit int

ast accepted this revision. ast added inline comments. This revision is now accepted and ready to land. Comment at: llvm/lib/Target/BPF/BTFDebug.cpp:1230 +Reloc == BPFCoreSharedInfo::BTF_TYPE_ID_REMOTE) OutMI.setOpcode(BPF::LD_imm64); else

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:124 +; CHECK: r0 = r2 +; CHECK: r0 = cmpxchg_64(r1 + 0, r0, r3) +; CHECK: encoding: [0xdb,0x01,0x00,0x00,0xf1,0x03,0x00,0x00] Looks like it's generating correct code without special

[PATCH] D72184: [WIP][BPF] support exchange/compare-and-exchange instruction

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:715 + +let Predicates = [BPFHasAtomicExt, BPFHasALU32], DecoderNamespace = "BPFALU32" in { + def XFADDW32 : XFALU32; i think -mcpu=v4 should include alu32. Otherwise the test matrix

[PATCH] D85174: BPF: simplify IR generation for __builtin_btf_type_id()

ast added a comment. Is it a cleanup or is it a fix for some bug? If latter there should be a new test for it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85174/new/ https://reviews.llvm.org/D85174

[PATCH] D83242: [clang][BPF] support expr with typedef/record type for FIELD_EXISTENCE reloc

ast added a comment. lgtm. curious what happens when type is defined within args, like: __builtin_preserve_field_info(*(struct { int a; } *)0, 2); Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83242/new/ https://reviews.llvm.org/D83242

[PATCH] D81479: [BPF] introduce __builtin_bpf_load_u32_to_ptr() intrinsic

ast added a comment. It feels that the same thing can be represented as inline asm. What advantage builtin has? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81479/new/ https://reviews.llvm.org/D81479

[PATCH] D81479: [BPF] introduce __builtin_load_u32_to_ptr() intrinsic

ast added inline comments. Comment at: clang/test/CodeGen/builtin-bpf-load-u32-to-ptr.c:5 +struct t { int a; int b; }; +void *test(struct t *arg) { return __builtin_load_u32_to_ptr(arg, 4); } + can it be expressed as: __builtin_load_u32_to_ptr(>b) ?

[PATCH] D74572: [BPF] preserve debuginfo types for builtin __builtin__btf_type_id()

ast accepted this revision. ast added inline comments. This revision is now accepted and ready to land. Comment at: llvm/lib/Target/BPF/BPFPreserveDIType.cpp:1 +//===- BPFPreserveType.cpp - Preserve DebugInfo Types -===// +// ast wrote: >

[PATCH] D74572: [BPF] preserve debuginfo types for builtin __builtin__btf_type_id()

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFPreserveDIType.cpp:1 +//===- BPFPreserveType.cpp - Preserve DebugInfo Types -===// +// BPFPreserveDIType.cpp Comment at:

[PATCH] D74668: [Clang][BPF] implement __builtin_btf_type_id() builtin function

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. lgtm. Thanks for explaining lvalue/value trick. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74668/new/ https://reviews.llvm.org/D74668

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. I've tested this patch on several kernel selftests/bpf/ and it works like magic. Very nice improvement. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69759

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. Great. Looks like inner propagation fix was straighforward. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. So what happens in the following case: struct S1; struct S2 { struct S1 *f; } relo *s2; // access s2->f struct S1 { int i; } relo; // access s2->f->i lack of relo on fwd declaration is not going to be sticky 'non-relo' when it's seen later, right? So

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. Is the attribute sticky with forward delcarations? struct s __reloc; struct s { int foo; } *s; what is s->foo ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69759

[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

ast added a comment. Looks great. This is a big improvement in usability. Is there a use case to apply that attribute to inner types automatically ? struct s1 { int foo; }; struct s2 { struct s1 *ptr; } __reloc__ *s2; s2->ptr->foo -- will both deref be relocatable or only

[PATCH] D69393: [RFC][DebugInfo] emit user specified address_space in dwarf

ast added a comment. > The address spaces envisioned by the Linux kernel appear to be more > informational and not descriptive of hardware characteristics. From the kernel pov the `__user` and normal are two different address spaces that must be accessed via different kernel primitives. User

[PATCH] D67980: [BPF] do compile-once run-everywhere relocation for bitfields

ast accepted this revision. ast added a comment. perfect. ship it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67980/new/ https://reviews.llvm.org/D67980 ___ cfe-commits mailing list

[PATCH] D67980: [BPF] do compile-once run-everywhere relocation for bitfields

ast added a comment. thanks for adding the tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67980/new/ https://reviews.llvm.org/D67980 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D67980: [CLANG][BPF] do compile-once run-everywhere relocation for bitfields

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. lgtm. thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67980/new/ https://reviews.llvm.org/D67980

[PATCH] D67980: [WIP][CLANG][BPF] do compile-once run-everywhere relocation for bitfields

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFCORE.h:17 + enum OffsetRelocKind : uint32_t { +FIELD_ACCESS_OFFSET = 0, +FIELD_EXISTENCE, why ACCESS_OFFSET is necessary? Isn't it the same as BYTE_OFFSET but for non-bitfield? May be single

[PATCH] D67883: [CLANG][BPF] permit any argument type for __builtin_preserve_access_index()

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. I tested few different examples. Generated code and relocations look good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67883/new/

[PATCH] D67734: [CLANG-BPF] change __builtin_preserve_access_index() signature

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. that's indeed much better. Thanks for adding all the tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67734/new/ https://reviews.llvm.org/D67734

[PATCH] D65615: [BPF] annotate DIType metadata for builtin preseve_array_access_index()

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. and the diff is shorter now :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65615/new/ https://reviews.llvm.org/D65615

[PATCH] D65615: [BPF] annotate DIType metadata for builtin preseve_array_access_index()

ast added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3402 QualType eltType, bool inbounds, - bool signedIndices, SourceLocation loc, + bool signedIndices,

[PATCH] D61809: [BPF] Preserve debuginfo array/union/struct type/access index

ast accepted this revision. ast added a comment. lgtm Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61809/new/ https://reviews.llvm.org/D61809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D61809: [BPF] Preserve debuginfo array/union/struct type/access index

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. from kernel, libbpf, tooling and user experience points of view this approach looks the best to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61809/new/

[PATCH] D61173: [BPF] do not generate predefined macro bpf

ast accepted this revision. ast added a comment. This revision is now accepted and ready to land. shipit Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61173/new/

[PATCH] D61173: [BPF] do not generate predefined macro bpf

ast added inline comments. Comment at: lib/Basic/Targets/BPF.cpp:23 MacroBuilder ) const { - DefineStd(Builder, "bpf", Opts); + Builder.defineMacro("__bpf"); + Builder.defineMacro("__bpf__"); I don't think anyone is using