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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
