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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


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


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:550-553
+  if (KnownLen == 0) {
+    MI.eraseFromParent();
+    return true;
+  }
----------------
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?


================
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;
+    }
----------------
I feel like this could be a helper, since it appears in a few of these functions?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:692-712
+  if (DstAlignCanChange) {
+    // Get an estimate of the type from the LLT.
+    Type *IRTy = getTypeForLLT(MemOps[0], C);
+    unsigned NewAlign = (unsigned)DL.getABITypeAlignment(IRTy);
+
+    // Don't promote to an alignment that would require dynamic stack
+    // realignment.
----------------
Could this be a helper, since it appears in two of these functions?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:825
+
+  LLVM_DEBUG(dbgs() << "Inlining memmove: " << MI << " into loads & stores\n");
+
----------------
Is this worth collecting a statistic for? Does SDAG have something like that for comparison?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:838
+    // Create the load.
+    Register LoadPtr;
+    Register Offset;
----------------
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`.


================
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;
----------------
initialize to false?


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65167