Fix for PR14824: Optimization arm_ldst_opt inserts newly generated instruction vldmia at incorrect position

[Jiangning Liu]

Apart from Renato's points, I have two more issues,

1) Can you combine those two statements,

+  for (unsigned i = At; i != 0;) {
+    --i;

to be

for (unsigned i = At; i != 0; --i) {

otherwise, I thought there might be other special changes to i in the loop.

2) It seems this piece of code change is unnecessary, because we needn't to
clean anything for a new chain, right?

@@ -1265,8 +1329,11 @@
         CurrSize = Size;
         CurrPred = Pred;
         CurrPredReg = PredReg;
+
+        unsigned Cleaned = CleanPrevLoadOpsInto(Reg, MemOps);
+
         MemOps.push_back(MemOpQueueEntry(Offset, Reg, isKill, Position,
MBBI));
-        ++NumMemOps;
+        NumMemOps += 1 - Cleaned;
         Advance = true;
       } else {

Thanks,
-Jiangning


2013/3/27 Stepan Dyatkovskiy <stpworld at narod.ru>

> Oh.. sorry, I've applied outdated patch, since I totally refactored it in
> Monday. Reattached.
> Relative to getElementaries... So you propose just to keep the list of
> unit registers currently used? right?  I try to fix it today.
> -Stepan.
> Renato Golin wrote:
>
>> On 25 March 2013 16:20, Stepan Dyatkovskiy <stpworld at narod.ru
>> <mailto:stpworld at narod.ru>> wrote:
>>
>>     Hello Jiangning,
>>     The main reason I did it, is that we need to analyze contents of
>>     MemOps *each time* we modify it: we need to prevent more than one
>>     "load" operations into the same place.
>>
>>
>> Hi Stepan,
>>
>> I like the idea of inspecting it on every load, but the current patch is
>> very inefficient.
>>
>> On:
>>
>> +    SmallVector<unsigned, 4> getElementaries(unsigned Reg) {
>>
>> You create the small vector on *every* call. This is really bad. And
>> then you iterate and compare every register of the new operations with
>> every register of every other operation so far. That's far from optimal.
>> This is why STL sorts and creates new sets, etc.
>>
>> If I get correctly you don't need a list of memory ops and their
>> respective registers, but the opposite, since you're looking for
>> registers, not memory ops.
>>
>> So, instead of keeping a list of operations and querying their registers
>> *every single time*, you can keep a list of registers "in use" with a
>> pointer to a small vector with the memory operations that use that
>> register.
>>
>> That will give you near constant time query over all registers in use
>> and you'll only have to gather the registers (via getElementaries) once
>> per operation.
>>
>> The rest looks ok, the encapsulation of MemOps into MemOpsTrckr seems to
>> work (wrt push_back/insert).
>>
>> Would also be good to have the IR in the PR bug as a test, just to make
>> sure the objective was met and to have a history on what this patch is
>> fixing.
>>
>> Thanks,
>> --renato
>>
>
>


-- 
Thanks,
-Jiangning
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130327/5bd69d52/attachment.html>