[PATCH] D66024: [SVFS] Vector Function ABI name demangler.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Not a review, but some thoughts nonetheless.



================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:64-66
+    return (ParamPos == Other.ParamPos && ParamKind == Other.ParamKind &&
+            LinearStepOrPos == Other.LinearStepOrPos &&
+            Alignment == Other.Alignment);
----------------
have you considered `std::tie()` ? Not sure if it would work/help here.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:71
+namespace VFABI {
+// Parse a string in the format
+// _ZGV<isa><mask><vlen><parameters>_<scalarname>[(<redirection>)].
----------------
Documentation commends have 3x `/`


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:75-82
+// \param VF -> holds <vlen> if <vlen> is a number.
+// \param IsMasked -> true if <mask> == "M", false if <mask> == "N".
+// \param IsScalable -> true if <vlen> == "x".
+// \param ISA -> maps to <isa>.
+// \param Parameters -> holds the <paramaters>.
+// \param ScalarName -> holds the <scalarname>.
+// \param VectorVarian -> holds <redirection> is present, otherwise holds
----------------
VectorVariant

But, i think this function should return `llvm::Optional<Some Struct>`, or `error_or` maybe


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:104
+  if (ParamKind == VFParamKind::Invalid)
+    llvm_unreachable("Unexpected Parameter Kind");
+  return ParamKind;
----------------
This is unreachable, right? Else i think this might need better error propagation strategy.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:164
+      // the case.
+      llvm_unreachable("Invalid Vector Function ABI name.");
+
----------------
same, and elsewhere.


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

https://reviews.llvm.org/D66024