On 10/24/19 3:23 PM, Jiong Wang wrote: > On Wed, Oct 23, 2019 at 3:27 AM Yonghong Song <yhs@xxxxxx> wrote: >> >> >> >> On 10/22/19 12:29 PM, Daniel Borkmann wrote: >>> On Mon, Oct 21, 2019 at 09:31:19PM -0700, Yonghong Song wrote: >>>> llvm alu32 was introduced in llvm7: >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_rL325987&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=0VCVs-aItkaVLRJ9Jp7YeX0We2JPKzcY7p_83Hlkso4&s=M0ANvh80tDNZb5JzE5vj9IETkKD87L1jFkcRHShC6Rk&e= >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_rL325989&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=0VCVs-aItkaVLRJ9Jp7YeX0We2JPKzcY7p_83Hlkso4&s=LABlrq9E6tmCwrbU2bCQa_LwchCaL8Tk5GczMCO5Cvs&e= >>>> Experiments showed that in general performance >>>> is better with alu32 enabled: >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lwn.net_Articles_775316_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=0VCVs-aItkaVLRJ9Jp7YeX0We2JPKzcY7p_83Hlkso4&s=qSDIIkauxw9Y_8rYH0AlvB4nvu06reDuhsb0GxSpoBo&e= >>>> >>>> This patch turned on alu32 with no-flavor test_progs >>>> which is tested most often. The flavor test at >>>> no_alu32/test_progs can be used to test without >>>> alu32 enabled. The Makefile check for whether >>>> llvm supports '-mattr=+alu32 -mcpu=v3' is >>>> removed as llvm7 should be available for recent >>>> distributions and also latest llvm is preferred >>>> to run bpf selftests. >>>> >>>> Note that jmp32 is checked by -mcpu=probe and >>>> will be enabled if the host kernel supports it. >>>> >>>> Cc: Jiong Wang <jiong.wang@xxxxxxxxxxxxx> >>>> Acked-by: Andrii Nakryiko <andriin@xxxxxx> >>>> Signed-off-by: Yonghong Song <yhs@xxxxxx> >>> >>> Applied, thanks! >>> >>> Would it make sense to include -mattr=+alu32 also into -mcpu=probe >>> on LLVM side or is the rationale to not do it that this causes a >>> penalty for various other, non-x86 archs when done by default >>> (although they could opt-out at the same time via -mattr=-alu32)? >> >> The current -mcpu=probe is mostly to provide whether particular >> instruction(s) are supported by the kernel or not. This follows >> traditional cpu concept. For -mattr=+alu32 case, instruction set >> remains the same, but we need to probe verifier capability. >> >> But I agree that for bpf probing verifier for alu32 support >> is totally reasonable. >> >> Jiong, could you help do an implementation in llvm side since >> you are more familiar with what alu32 capability needs to be >> checked for verifier? Thanks! > > I think alu32 code-gen becomes good and stable after jmp32 > instructions (cpu=v3) supported, so if we want to enable alu32 at > default, perhaps could just link it with v3 probe, and also Daniel's > opt-out suggestion makes sense. > > Will try to do one impl but not sure could catch the timeline > tomorrow. For what it's worth, tomorrow will be my last day using > Netronome email, I will use wong.kwongyuan.tools@xxxxxxxxx for > bpf/kernel contributing temporarily. Jiong, thanks for letting us know. Maybe the following diff will be okay? diff --git a/llvm/lib/Target/BPF/BPFSubtarget.cpp b/llvm/lib/Target/BPF/BPFSubtarget.cpp index ab3452501b9..f3cb03b1f1f 100644 --- a/llvm/lib/Target/BPF/BPFSubtarget.cpp +++ b/llvm/lib/Target/BPF/BPFSubtarget.cpp @@ -52,6 +52,7 @@ void BPFSubtarget::initSubtargetFeatures(StringRef CPU, StringRef FS) { if (CPU == "v3") { HasJmpExt = true; HasJmp32 = true; + HasAlu32 = true; return; } } Considering in general -mattr=+alu32 improves code size and performance. I don't think there is need to disable HasAlu32. Any regression we should just debug and fix. What do you think? Yonghong
