[PATCH] D69976: [VFABI] Read/Write functions for the VFABI attribute.

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 17 inline comments as done.
fpetrogalli added inline comments.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1172
+#ifndef NDEBUG
+  const Module *M = CI.getModule();
+#endif
----------------
sdesmalen wrote:
> The code would be simpler to read if we let the compiler hoist `M` out of the loop below, so please move it to the second `NDEBUG` block.
I invoked `getModule` directly in the assertion, as it is the only place where it is used.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:304
+  Out << "_ZGV" << _LLVM_TLI_ << "N" << VF;
+  for (unsigned I = 0; I < CI->getNumArgOperands(); ++I) {
+    Out << "v";
----------------
sdesmalen wrote:
> nit: unnecessary curly braces.
This function shouldn't be here, I have removed it.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:327
+                                  const StringRef VFName) {
+  assert(CI && "Invalid CallInst.");
+  Module *M = CI->getParent()->getParent()->getParent();
----------------
sdesmalen wrote:
> nit: is this assert really necessary?
This method shouldn't be here. I have removed it.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:353
+void VFABI::addMappingsFromTLI(const TargetLibraryInfo *TLI, CallInst *CI) {
+  assert(TLI && "Invalid TLI.");
+  assert(CI && "Invalid CallInst.");
----------------
sdesmalen wrote:
> are these asserts necessary?
This function shouldn't be here. I have removed it.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:365
+  const std::string ScalarName = CI->getCalledFunction()->getName();
+  // Nothing to be done if the TLI things the function is not
+  // vectorizable.
----------------
sdesmalen wrote:
> nit: the comment doesn't really clarify more than the code already expresses, so can be removed.
This function shouldn't be here. I have removed it.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:376
+    const std::string TLIName = TLI->getVectorizedFunction(ScalarName, VF);
+    if (TLIName != "") {
+      std::string MangledName = mangleTLIName(TLIName, CI, VF);
----------------
sdesmalen wrote:
> `if (!TLIName.empty())`
This function shouldn't be here. I have removed it.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:378
+      std::string MangledName = mangleTLIName(TLIName, CI, VF);
+      //      List.push_back(MangledName);
+      SetOfMangledNames.insert(MangledName);
----------------
sdesmalen wrote:
> remove?
This function shouldn't be here. I have removed it.


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

https://reviews.llvm.org/D69976