[PATCH] D66024: Name Demangling as specified in the Vector Function ABI

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for splitting this functionality off into a separate patch!
I added some comments inline, but my main points are:

- that it would be better to parse the mangled string incrementally, rather than extracting each feature from the string individually.
- if you change this patch to parse incrementally, interfaces such as `getIsMasked` or `getISA` should probably be private methods to VectorFunctionShape, and be renamed to `parseIsMasked` and `parseISA`.



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:149
+/// \param Result True if the MangledName follows the ABI signature
+bool getABISignature(StringRef MangledName);
+/// Gets Instruction Set Architecture
----------------
I don't think we want to expose any of these interfaces directly, as they are only relevant to the parsing of the mangled name vis-a-vis the construction of the VectorFunctionShape object. After it has been parsed, the VectorFunctionShape object can be queried directly for these properties.

If you make them private methods to VectorFunctionShape, then a constructor of VectorFunctionShape can take `StringRef MangledName` and parse the string to fill out the object, calling all these interfaces under the hood. For this reason, I think you'll want to move `demangleName` and `getParameters` from D66025 to this patch as well.

I'd also recommend renaming these functions to use 'parse' instead of 'get', and instead of returning the value that they have parsed directly, you can wrap it in a `Optional<>`, so you can check whether an issue occurred while parsing the mangled name.


================
Comment at: llvm/lib/Analysis/SearchVectorFunctionSystem.cpp:52
+
+unsigned VFABI::getVF(StringRef MangledName) {
+  // Capture the VF
----------------
If you implement these methods as 'parse' methods, you can simplify the code by just updating the StringRef for MangledName. e.g.

  // note that MangledName is now a reference, so when the value
  // is parsed it is sliced off from the name.
  Optional<bool> parseMask(StringRef &MangledName) { 
    bool Mask;
    switch (MangledName[0]) {
    case 'N':
      Mask = false;
      break;
    case 'M' :
      Mask = true;
      break;
    default:
      // parse failure, return nothing (caller can choose to
      // error or recover). MangledName is not updated.
      return {};
    }
    MangledName = MangledName.drop_front(1);
    return {Mask};
  }

for parseVF, you can do something along the lines of:

  Optional<unsigned> parseVF(StringRef &MangledName) {
    // Get the string containing the VF
    StringRef VFString = MangledName.take_until([](char c) { return ::isdigit(c); });

    unsigned VF;
    if (VFString.consumeInteger(10, VF))
      return {}; // Could not parse as integer

    // Successfully parsed, so update MangledName.
    MangledName.drop_front(VFString.size());
    return {VF};
  }


================
Comment at: llvm/unittests/Analysis/SearchVectorFunctionSystemTest.cpp:1
+//===------- SearchVectorFunctionSystemTest.cpp - SVFS Unittests  ---------===//
+//
----------------
Do you want to rename this test to something like VectorFunctionABITest.cpp ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66024