[PATCH] D10676: ARMLoadStoreOpt: Merge subs/adds into LDRD/STRD; Factor out common code

[Matthias Braun]

MatzeB added a comment.

Thanks for the review, comments below:


================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:1123
@@ +1122,3 @@
+static MachineBasicBlock::iterator
+findIncDecAfter(MachineBasicBlock::iterator MBBI, unsigned Reg,
+                ARMCC::CondCodes Pred, unsigned PredReg, int &Offset) {
----------------
rengolin wrote:
> Isn't there a way to merge these two functions in one?
Actually in some intermediate versions I had merged the two. The problem is however that after finding a incdec before you have to decide on different factors whether you can actually fold that increment/decrement in and if you cannot you go on searching one after.
These factors however vary between each of the three users, so to merge the two functions you need to pass in some predicate lambdas or bitsets of which inc/decs are allowed. That turned out to be messier than simply having two functions.

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:1772
@@ -1759,1 +1771,3 @@
   Candidates.clear();
+  for (MachineInstr *MI : MergeBaseCandidates)
+    MergeBaseUpdateLSDouble(*MI);
----------------
rengolin wrote:
> What if the patterns you're merging up on line 1756 are also in this list? Shouldn't MergeBaseUpdateLSDouble() remove the match from the list and then you won't need to do this? Or is this what the test on "Imm != 0 -> return" on line 1365 is doing?
I'm not sure I understand your concerns. In anyway this second loop and the MergeBaseCandidates list requires a comment (which I will add): Currently there is a LDRD/STRD formation pass that runs before register allocation. Unfortunately I cannot disable that one yet, as it is less dependent on the schedule and manages to catch some cases which the late peephole can't when the loads/stores aren't scheduled next to each other.
Anyway the previously formed LDRD/STRD instructions will end up in this list, the ones that are formed by the MergeOpsUpdate() are handled on line 1750.

In the long term I would like to disable the pre-RA formation of LDRD/STRD but that would require to enhance the ARMLoadStoreOpt pass to reschedule instruction. I've actually worked on that but could not get it into a state yet where it doesn't occasionally perform an invalid transformation.


rL LLVM

http://reviews.llvm.org/D10676