[PATCH] D67161: [clang,ARM] Initial ACLE intrinsics for MVE.

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

Sorry. I wasn't ignoring this (sort-of), I just knew that you were on holiday and this is a bit of a big one.

But I like this. I still have to take a deeper look into the main tablegen parts, but it looks very powerful.

I presume from here adding new intrinsics is mostly a case of adding to arm_mve.td (with perhaps some minor parts in defs and maybe some custom bits here and there).



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8529
   "argument should be a multiple of %0">;
+def err_argument_not_power_of_2 : Error<
+  "argument should be a power of 2">;
----------------
Do we have any/should we have some tests for these errors?


================
Comment at: clang/include/clang/Basic/arm_mve_defs.td:266
+// vidupq_wb_u16 -> vidupq_u16.
+def PNT_WB:     PolymorphicNameType<0, "wb">;
+
----------------
These are not used yet, right? They are not meant for the vldr gathers, those don't have polymorphic names (if I'm reading this list of intrinsics right). These are for vidup's as the comment says?


================
Comment at: clang/test/CodeGen/arm-mve-intrinsics/scalar-shifts.c:2
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8.1m.main+mve.fp -mfloat-abi=hard -O3 -S -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8.1m.main+mve.fp -mfloat-abi=hard -DPOLYMORPHIC -O3 -S -emit-llvm -o - %s | FileCheck %s
----------------
These tests all run -O3, the entire pass pipeline. I see quite a few tests in the same folder do the same thing, but in this case we will be adding quite a few tests. Random mid-end optimizations may well keep on altering them.

Is it possible to use -disable-O0-optnone and pipe them into opt -mem2reg? What would that do to the codegen, would it be a lot more verbose than it is now?

Also could/should they be using clang_cc1?


================
Comment at: clang/test/CodeGen/arm-mve-intrinsics/scalar-shifts.c:3
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8.1m.main+mve.fp -mfloat-abi=hard -O3 -S -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8.1m.main+mve.fp -mfloat-abi=hard -DPOLYMORPHIC -O3 -S -emit-llvm -o - %s | FileCheck %s
+
----------------
POLYMORPHIC isn't used here. Same for vcvt below (Is there a polymorphic vcvt?)


================
Comment at: clang/test/CodeGen/arm-mve-intrinsics/vminvq.c:12
+//
+uint32_t test_vminvq_u32(uint32_t a, uint32x4_t b)
+{
----------------
Its probably worth trying to fill in tests for most types.


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:82
+#if 0
+} // stop emacs from wanting to auto-indent everything to 2 spaces inside here
+#endif
----------------
Is this needed still? It seems like something that should be handled in clang-format/emacs directly.


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:1403
+        " *\n"
+        " * Permission is hereby granted, free of charge, to any person "
+        "obtaining a copy\n"
----------------
Clang format really made a mess of this, didn't it.

Is this is old license? Should it be updated to the new one. I imagine that these generated headers might well have been missed in the switchover.


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:132
+    // the pointee type.
+    Pointer,
+  };
----------------
The gathers are really a Vector of Pointers, in a way. But in the intrinsics (and IR), they are just treated as a vector of ints, so I presume a new type is not needed? We may (but not yet) want to make use of the llvm masked gathers. We would have to add codegen support for them first though (which I don't think we have plans for in the near term).


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:520
+  // time.
+  bool needs_visiting(unsigned Pass) {
+    bool ToRet = Visited < Pass;
----------------
needsVisiting.

I would guess the same for other places that use snake_case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67161