[PATCH] Fix bug in llvm::DemoteRegToStack

[Nick Lewycky]

--- lib/Transforms/Utils/DemoteRegToStack.cpp
+++ lib/Transforms/Utils/DemoteRegToStack.cpp
@@ -21,6 +21,7 @@
  /// alloca.  This allows the CFG to be changed around without fear of
  /// invalidating the SSA information for the value.  It returns the 
pointer to
  /// the alloca inserted to create a stack slot for I.
+
  AllocaInst *llvm::DemoteRegToStack(Instruction &I, bool VolatileLoads,
                                     Instruction *AllocaPoint) {
    if (I.use_empty()) {

Remove the extra blank line, this comment is attached to this function.

+  StoreInst *Store = new StoreInst(&I, Slot, InsertPt);
+
+  // Change all of the users of the instruction, except the Store we've 
just
+  // inserted, to read from the stack slot.
+  SmallVector<User*, 4> UsersOfI(I.users());
+
+  for (User *U : UsersOfI) {
+    if (PHINode *PN = dyn_cast<PHINode>(U)) {

Can you avoid the extra copy? What if you create the store now, but 
don't insert it yet? (I think InsertPt can't be one of the users, correct?)

While we're here:

   for (; isa<PHINode>(InsertPt) || isa<LandingPadInst>(InsertPt); 
++InsertPt)
     /* empty */;   // Don't insert before PHI nodes or landingpad instrs.

This is redundant with getFirstInsertionPt. Can this be hoisted up to 
the only place that sets InsertPt without using getFirstInsertionPt?

Nick

Akira Hatanaka wrote:
> In the following example, when DemoteRegToStack demotes %call.i.i.i14.i.i, it firsts scans all the uses of %call.i.i.i14.i.i and inserts load instructions before all its users. Since %0 is a phi instruction, a load instruction is inserted in the predecessor block, which is the entry basic block. Then it inserts a store instruction to save the value it is demoting to the stack. In this case, %call.i.i.i14.i.i is an invoke, so it creates a basic block that splits the critical edge (entry-do.body.i.i.i), and inserts the store there. This is incorrect because the store instruction should be executed before the load instruction.
>
> define void @_Z4foo1c(i8 signext %a) {
> entry:
>    ...
>    %call.i.i.i14.i.i = invoke noalias i8* @_Znwm(i32 1024)
>            to label %do.body.i.i.i unwind label %lpad.body
>
> do.body.i.i.i:  ; preds = %entry, %_ZNSt3__116allocator_traitsINS_9allocatorIcEEE9constructIccEEvRS2_PT_RKT0_.exit.i.i.i
>    %0 = phi i8* [ %incdec.ptr.i.i.i, %_ZNSt3__116allocator_traitsINS_9allocatorIcEEE9constructIccEEvRS2_PT_RKT0_.exit.i.i.i ], [ %call.i.i.i14.i.i, %entry ]
>    ...
>
> The IR after SjLjEHPrepare pass looks like this:
>
> define void @_Z4foo1c(i8 signext %a) {
> entry:
>    %call.i.i.i14.i.i.reg2mem = alloca i8*
>     ...
>    %call.i.i.i14.i.i.reload = load volatile i8** %call.i.i.i14.i.i.reg2mem
>    ...
>    %call.i.i.i14.i.i = invoke noalias i8* @_Znwm(i32 1024)
>            to label %entry.do.body.i.i.i_crit_edge unwind label %lpad.body
>
> entry.do.body.i.i.i_crit_edge:                    ; preds = %entry
>    store i8* %call.i.i.i14.i.i, i8** %call.i.i.i14.i.i.reg2mem
>    br label %do.body.i.i.i
>
> do.body.i.i.i:                                    ; preds = %entry.do.body.i.i.i_crit_edge, %_ZNSt3__116allocator_traitsINS_9allocatorIcEEE9constructIccEEvRS2_PT_RKT0_.exit.i.i.i
>    %3 = phi i8* [ %incdec.ptr.i.i.i, %_ZNSt3__116allocator_traitsINS_9allocatorIcEEE9constructIccEEvRS2_PT_RKT0_.exit.i.i.i ], [ %call.i.i.i14.i.i.reload, %entry.do.body.i.i.i_crit_edge ]
>
> This patch fixes the bug by changing llvm::DemoteRegToStack to insert the store instruction and create basic blocks that split critical edges first, and then insert the instructions to reload the value.
>
> http://reviews.llvm.org/D6729
>
> Files:
>    lib/Transforms/Utils/DemoteRegToStack.cpp
>    test/CodeGen/ARM/sjlj-prepare-critical-edge.ll
>
> EMAIL PREFERENCES
>    http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits