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

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked 8 inline comments as done.
aemerson added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:442
+  // not need to be loaded.
+  if (!(SrcAlign == 0 || SrcAlign >= DstAlign))
+    return false;
----------------
paquette wrote:
> de morgan?
Done.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:550-553
+  if (KnownLen == 0) {
+    MI.eraseFromParent();
+    return true;
+  }
----------------
paquette wrote:
> Could this part be pulled out into the caller? It shows up in each of the optimization functions and does the same thing. Then this could just assert on KnownLen?
Sure.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:580-586
+    if (NewAlign > Align) {
+      unsigned FI = FIDef->getOperand(1).getIndex();
+      // Give the stack frame object a larger alignment if needed.
+      if (MFI.getObjectAlignment(FI) < NewAlign)
+        MFI.setObjectAlignment(FI, NewAlign);
+      Align = NewAlign;
+    }
----------------
paquette wrote:
> I feel like this could be a helper, since it appears in a few of these functions?
I'm gonna disagree here because the blocks use 6 variables from the function scope, so by the time we plumb through all the arguments we're going to end up with a similar amount of code duplication, with additional indirection.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:825
+
+  LLVM_DEBUG(dbgs() << "Inlining memmove: " << MI << " into loads & stores\n");
+
----------------
paquette wrote:
> Is this worth collecting a statistic for? Does SDAG have something like that for comparison?
I don't think SelectionDAG does either. I can add it if required but these are so common I'm not sure what the benefit will be to have them.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:838
+    // Create the load.
+    Register LoadPtr;
+    Register Offset;
----------------
paquette wrote:
> If you init this to Src, then you can kill the `if` and make the `else` a `if (CurrOffset != 0)`?
> 
> Then you can also scope Offset into the new `if`.
Ok.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:880
+  Register Src = 0, Dst = 0, Len = 0, Val = 0;
+  bool IsVolatile;
+  unsigned DstAlign = 0, SrcAlign = 0;
----------------
paquette wrote:
> initialize to false?
Ok.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:885-886
+  switch (ID) {
+  case Intrinsic::memcpy:
+  case Intrinsic::memmove:
+    Dst = MI.getOperand(1).getReg();
----------------
paquette wrote:
> Should we just assert that we have one of these IDs instead of putting them in a switch?
I'll simplify it a bit.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:907-909
+  // Don't try to optimize volatile.
+  if (IsVolatile)
+    return false;
----------------
paquette wrote:
> Could probably check this before entering the switch
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65167