[PATCH] D102118: [BPF] add support for 32 bit registers in inline asm

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song added a comment.

Generally looks good. One suggestion and a few nits. Thanks!



================
Comment at: clang/lib/Basic/Targets/BPF.cpp:54
+    if (Feature == "+alu32") {
+      HasAlu32 = true;
+    }
----------------
-mcpu=v3 also enables alu32. See below llvm code
```
void BPFSubtarget::initSubtargetFeatures(StringRef CPU, StringRef FS) {
  if (CPU == "probe")
    CPU = sys::detail::getHostCPUNameForBPF();
  if (CPU == "generic" || CPU == "v1")
    return;
  if (CPU == "v2") {
    HasJmpExt = true;
    return;
  }
  if (CPU == "v3") {
    HasJmpExt = true;
    HasJmp32 = true;
    HasAlu32 = true;
    return;
  }
}
```

Could you add a check with setCPU() to set HasAlu32 = true if it is v3? This way, when people directly use -mcpu=v3 in clang command line, 'w' constraint is also supported.


================
Comment at: clang/lib/Basic/Targets/BPF.cpp:60
+}
\ No newline at end of file

----------------
empty line here.


================
Comment at: clang/test/CodeGen/bpf-inline-asm.c:6
+
+void test_generic_constraints(int var32, long var64) {
+  asm("%0 = %1"
----------------
I assume test_generic_constraints() will pass even without this patch, but it is a good addition for clang side of bpf inline-asm support, so I think it is okay to be included in this patch.


================
Comment at: clang/test/CodeGen/bpf-inline-asm.c:33
+}
\ No newline at end of file

----------------
empty line at the end of file.


================
Comment at: llvm/test/CodeGen/BPF/inlineasm-wreg.ll:1
+
+; RUN: llc < %s -march=bpfel -mattr=+alu32 -verify-machineinstrs | FileCheck %s
----------------
Let us remove the empty line in the top of the test file.


================
Comment at: llvm/test/CodeGen/BPF/inlineasm-wreg.ll:20
+}
\ No newline at end of file

----------------
empty line at the end of file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102118/new/

https://reviews.llvm.org/D102118