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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D66024#1668515 <https://reviews.llvm.org/D66024#1668515>, @fpetrogalli wrote:

> I have updated the patch according to your feedback. In particular, I have modified all the parsing method to use the `ParseRet` enum instead of booleans. It looks better now, thanks.


Agreed, this looks good now, thanks!

> Once outstanding work is the comparison operator of `VFShape`. I have proposed a solution in a comment, let me know what you think, I will apply it if you agree on the approach. BTW: good catch. The VFShape should carry information only about the shape, not about the names of the functions.

Yes, that seems like a good suggestion.

> One more nit: while working on the SVFS, I have noticed that the `llvm_unreachable` I have added at the end of the static method to create FShape out of the parsed data was quite annoying: it got fired every time the CI I was trying to vectorize didn't have the name with the mangled attribute. hence, I have removed the assertion, and converted the `ASSERT_DEATH` tests into `EXPECT_FALSE` tests in `TEST(VectorFunctionABITests, OnlyValidNames)`. This should not be controversial (I am happy to be told I am wrong if that's the case).

That seems like the better choice given how you've shaped the interface that returns a `Optional<VFShape>`, but I don't really understand why this would break CI.


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

https://reviews.llvm.org/D66024