Re: [PATCH bpf-next v2] tools/bpf: turn on llvm alu32 attribute by default

[Yonghong Song <yhs@fb.com>]

On 10/24/19 3:23 PM, Jiong Wang wrote:
> On Wed, Oct 23, 2019 at 3:27 AM Yonghong Song <yhs@fb.com> 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@netronome.com>
>>>> Acked-by: Andrii Nakryiko <andriin@fb.com>
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>
>>> 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@gmail.com 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