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

[Dave Green via Phabricator via cfe-commits]

dmgreen added inline comments.


================
Comment at: clang/include/clang/Basic/arm_mve_defs.td:119
+// The type Void can be used for the return type of an intrinsic, and as the
+// parameter type for intrinsics that aren't actually parametrised by any kind
+// of _s32 / _f16 / _u8 suffix.
----------------
parametrised->parameterised


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:207
+    // pointer, especially if the pointee is also const.
+    if (isa<PointerType>(Pointee)) {
+      if (Const)
----------------
Would making this always east const be simpler? Do we generate anything with inner pointer types? Should there be a whitespace before the "const " in Name += "const "?


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:251
+  }
+  virtual unsigned sizeInBits() const override { return Bits; }
+  ScalarTypeKind kind() const { return Kind; }
----------------
These don't need to be virtual and override, they can just be override. Same elsewhere.


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:325
+    return Element->c_name_base() + "x" + utostr(Registers);
+  }
+
----------------
How come this doesn't have/need a llvm_name? Are they just all handled manually?


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:382
+// ones are the same across the whole set (so that no variable is needed at
+// all).
+//
----------------
Cool.

Do you know how much this helps? Do the benefits outweigh the costs in the complexity of this code?

And do you know, once we have added a lot of instruction how long this will take to run (I know, difficult question to answer without doing it first, but is it roughly O(n log n)? And memory will never be excessive? A very unscientific test I just ran made it seem pretty quick, but that was obviously without the number of intrinsics we will have in the end). We should try and ensure that we don't make this a compile time burden for llvm.


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:455
+
+class Value {
+public:
----------------
Is it worth giving this a different name to more obviously distinguish it from llvm::Value?


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:723
+    CodeGenParamAllocator DummyParamAlloc;
+    V->gen_code(nulls(), DummyParamAlloc);
+
----------------
What is this doing?


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:755
+    }
+    std::list<Value::Ptr> used;
+    gen_code_dfs(Code, used, Pass);
----------------
Used

And why a list, out of interest?


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:1023
+    Value::Ptr Arg = getCodeForDagArg(D, 0, Scope, Param);
+    if (const auto *ST = dyn_cast<ScalarType>(CastType)) {
+      if (!ST->requires_float()) {
----------------
Do you envision this may require float const or vector casts in the future?


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:1125
+  if (!PolymorphicNameType->isValueUnset("ExtraSuffixToDiscard")) {
+    std::string ExtraSuffix =
+        PolymorphicNameType->getValueAsString("ExtraSuffixToDiscard");
----------------
Some of these could use StringRef's more liberally, here and elsewhere.


================
Comment at: clang/utils/TableGen/MveEmitter.cpp:1595
+      // Is this parameter the same for all intrinsics in the group?
+      bool Constant = true;
+      auto it = kv.second.begin();
----------------
Can this use (something like):
bool Constant = llvm::all_of(kv.second, [&](const auto &OI) { return OI.ParamValues[i] == kv.second[0].ParamValues[i]; } ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67161