[PATCH] D65167: [GlobalISel] Support for inlining memcpy, memset and memmove calls

[Jessica Paquette via Phabricator via llvm-commits]

paquette accepted this revision.
paquette added a comment.
This revision is now accepted and ready to land.

LGTM with some comments



================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:428
+
+// Returns a list of types to use for memory op lowering in MemOps. An partial
+// port of findOptimalMemOpLowering in TargetLowering.
----------------
s/an partial/a partial/


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:595
+
+  // Generate the stores.
+  LLT PtrTy = MRI.getType(Dst);
----------------
Can we have some documentation for what the memset is being optimized to and what the stores should look like?

(same for the other optimize functions)


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:880
+    return false; // Leave it to the legalizer to lower it to a libcall.
+  unsigned KnownLen = LenVRegAndVal->Value;
+
----------------
IIRC this returns an int64_t and can return negative values; should we check that this is positive? Or is it not possible for it to be negative?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:882-885
+  if (KnownLen == 0) {
+    MI.eraseFromParent();
+    return true;
+  }
----------------
should we assert that KnownLen != 0 in the optimize functions now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65167