[PATCH] D37298: [ForwardOpTree] Allow forwarding in the presence of region statements

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM, I have no more correctness concerns.

In https://reviews.llvm.org/D37298#857643, @grosser wrote:

> The getLI is moved to make it public. It  was not public before.


I don't see any new uses of it that'd require it to be public.



================
Comment at: lib/Support/VirtualInstruction.cpp:186-189
+    for (Instruction *Inst : Stmt->getInstructions())
+      RootInsts.emplace_back(Stmt, Inst);
+    for (BasicBlock *BB : Stmt->getRegion()->blocks())
+      if (Stmt->getRegion()->getEntry() != BB)
----------------
This is correct, but did you consider not adding the instruction list instructions as roots? Simplify would be able to remove them and their dependencies if they are not considered roots.


================
Comment at: test/ForwardOpTree/forward_from_region.ll:54-59
+; CHECK-NEXT:     Instructions copied: 1
+; CHECK-NEXT:     Known loads forwarded: 0
+; CHECK-NEXT:     Read-only accesses copied: 0
+; CHECK-NEXT:     Operand trees forwarded: 1
+; CHECK-NEXT:     Statements with forwarded operand trees: 1
+; CHECK-NEXT: }
----------------
Using `CHECK:` would make the test less fragile if we want to add more stats.


================
Comment at: test/ForwardOpTree/noforward_from_region.ll:3
 ;
-; Do not move instructions to region statements.
+; Move instructions from region statements.
 ;
----------------
The comment is inaccurate. The load is not moved. Mentioning the reason for this would also be nice.


================
Comment at: test/ForwardOpTree/noforward_from_region.ll:17
 ;
-define void @func(i32 %n, double* noalias nonnull %A) {
+define void @func(i32 %n, double* noalias nonnull %A, double* noalias nonnull %B) {
 entry:
----------------
Array `%B` is not used.


================
Comment at: test/ForwardOpTree/noforward_from_region.ll:33
+      %x = load double, double* %A
+      store double %x, double* %A
       br label %bodyB
----------------
The pseudocode summary says it's `A[0] = 42`, but this is `A[0] = A[0]`. I'd prefer the former because it's one less (irrelevant) memory accesses.


https://reviews.llvm.org/D37298