[PATCH] Scalar/PHI code genration

[Johannes Doerfert]

I will change the stuff according to the comments (including the spelling) and then commit it.


================
Comment at: lib/Analysis/ScopInfo.cpp:888
@@ -895,1 +887,3 @@
+      MemAccs.back()->setNextMA(MA);
+    MA = MemAccs.back();
   }
----------------
grosser wrote:
> Is this correct? In case we add three memory accesses A,B,C. Would this code not first add A to the map, than add a link from A to B, and then overwrite the link to B with C, which results in us loosing the link to B entirely?
> 
> I somehow feel the manually implemented linked list adds unnecessary complexity and also somehow mixes the MemoryAccess definition and
> the InstructionToAccess mapping. I wonder if it might not be clearer to make the InstructionToAccess Map a mapping from instructions to a
> vector of accesses.
We add A and MA is null.

We add B and MA is now A (as it was already in the Inst2Acc map)
As MA is not null we link B (the last access) to A:
  B -> A
and change the mapping in Inst2Acc to B:
  Inst2Acc[AccessInst] ==> B

This way nothing is lost and in the end it looks like:
  Inst2Acc[AccessInst] ==> C -> B -> A


We could map it to a vector but I thought the overhead would be unproportional.  If you think it is worth it I can change it again.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:278
@@ -259,1 +277,3 @@
+    Instruction *NewLoad =
+        generateScalarLoad(Stmt, Load, BBMap, GlobalMap, LTS);
     // Compute NewLoad before its insertion in BBMap to make the insertion
----------------
grosser wrote:
> As this function only returns a Value, there is no need to change this to 'Instruction' (similar comment below). To not distract from the core change, I would suggest to avoid this change (or perform it in a separate patch).
Ok.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:360
@@ +359,3 @@
+    assert((InstCopy == nullptr || BBMap[&Inst] == InstCopy) &&
+           "Bad copy mapping found");
+  }
----------------
grosser wrote:
> This patch modifies various functions to pass the newly created instruction on.  I somehow believe to remember you earlier passed these on for some other use
> case, but the current patch only seems to need these changes for the assert.
> 
> If the assert has shown helpful during development, it seems reasonable to commit it (and the associated changes). However, in the optimal case, this would be a separate patch committed ahead of time. If that is to much work, I do not insist on separating this out.
> 
> Also, a general phabricator issue is that the updated commit messages are not visible. It would be nice if you could add a comment in the commit message that explains the intention of these set of changes (to help people understand that they are not strictly necessary for the core goal of this patch).
This might be true, I'll check if I can revert the return Instruction/Value part.

http://reviews.llvm.org/D7513

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/