[PATCH] D68941: [ScopBuilder]Fix bug 38358 by preserve correct order of ScopStmts

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I remember I cared about the epilogue being the last, which is the reason for the reversed list. It ensures that the terminator is always visited first, hence added first into the list, hence being the last statement. (I am a bit surprised that no test fails). An example could be:

  %b = load %B  // merged with terminator by joinOperandTree
  store 42 // independent statement
  br %b // terminator, epilogue

Seeing this example, it is clear that `load %B` (and therefore the epilogue) cannot be put after `store 42`, as it might store to `%A`. That is, the epilogue is not necessarily always the last instruction. It should not matter since the PHI-writes have their own memory locations that do not alias with other memory.
[^^ just my thoughts about whether everything is sound; I could not find an issue]

Looks correct, Thanks for the patch!
Do you want me to commit this for you?



================
Comment at: lib/Analysis/ScopBuilder.cpp:2125-2126
 
+  // Order of instruction groups needs to be preserved wrto the original order
+  // of ordered instructions in basic block, rather than order of leadership
+  // instructions.  We scan the ordered instructions and insert elements for
----------------
[grammar/suggestion] The order of statements must be preserved w.r.t. their ordered instructions. Without this explicit scan, we would also use non-ordered instructions (whose order is arbitrary) to determine statement order.


================
Comment at: lib/Analysis/ScopBuilder.cpp:2137
+
+    // Insert elment for the leader instruction.
+    (void)LeaderToInstList[*LeaderIt];
----------------
[typo] elment


================
Comment at: test/ScopInfo/preserve-equiv-class-order-in-basic_block.ll:66-68
+attributes #0 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind }
----------------
[suggestion] Can you try to reduce unnecessary parts? Such as: function attributes, instruction attributes ("!tbba"), comdat, module metadata (`!llvm.module.flags`, `!llvm.ident`). Function parameters are nicer than globals, but more work to convert.


Repository:
  rPLO Polly

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68941/new/

https://reviews.llvm.org/D68941