[PATCH] D13239: [ARM] Modify codegen for memcpy intrinsic to prefer LDM/STM.

[James Molloy via llvm-commits]

jmolloy added a subscriber: jmolloy.
jmolloy requested changes to this revision.
jmolloy added a reviewer: jmolloy.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Scott,

In general I like this change. It's an elegant solution to the problem. I've got a bikeshed to paint regarding the name - I think MEMCPY might be more appropriate as that's exactly what it's doing.

I think there should be more documentation about the inputs and outputs of the node - it took me a bit to realise that the outputs are the updated base registers (which means you can chain them - neat!)

There also looks to be minimal-zero non-Thumb1 tests for this - this happens in all modes so I'd expect the same amount of testing for each of ARM, T1 and T2.

The instprinter stuff is ugly. But I see why it's needed.

Cheers,

James


================
Comment at: lib/Target/ARM/ARMISelLowering.h:190
@@ +189,3 @@
+      // instructions.
+      MCOPY,
+
----------------
Can we call this MEMCPY instead of MCOPY? It self-describes a bit better, I think.

================
Comment at: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp:752
@@ +751,3 @@
+  // it now.
+  std::vector<MCOperand> RegOps(MI->size() - OpNum);
+  std::copy(MI->begin() + OpNum, MI->end(), RegOps.begin());
----------------
You should be able to construct the vector using the iterator construction syntax:

    std::vector<MCOperand> RegOps(MI->begin() + OpNum, MI->end());

================
Comment at: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp:754
@@ +753,3 @@
+  std::copy(MI->begin() + OpNum, MI->end(), RegOps.begin());
+  std::sort(RegOps.begin(), RegOps.end(),
+            [this](const MCOperand &O1, const MCOperand &O2) -> bool {
----------------
Use std::stable_sort instead of std::sort, for deterministicness.


http://reviews.llvm.org/D13239