Make globalaa-retained.ll test catching more cases.

[a.elovikov]

Summary

• Add checks for store. That is needed because GlobalsAA is called twice in the current pipeline with different sets of Function passes following it. However, the loads are eliminated using instcombine which happens everywhere. On the other hand, DeadStoreElimination is performed only once so by checking for store we'll be able to catch more cases when GlobalsAA is invalidated unintentionally.
• Add empty function above/below the test so that we don't depend on the relative order of instcombine/dead-store-elimination and the pass that invalidates the analysis (inside the same FunctionPassManager).

[kristof.beyls]

llvm/test/Transforms/PhaseOrdering/globalaa-retained.ll

9–14 ↗

(On Diff #95127)

I think this comment is a bit hard to understand. Below I tried tweaking the comment a bit, so it's a bit easier to understand, just adding more words to point to key ideas needed to understand the comment. I've marked my additions between asterisks.
Is the comment still valid? If so, I think it'll be easier to understand.

; *This test checks that a number of loads and stores are eliminated, that can only be eliminated based on GlobalsAA information. As such, it tests that GlobalsAA information is retained until the passes that perform this optimization, and it protects against accidentally dropping the GlobalsAA information earlier in the pipeline, which has happened a few times.*
; *GlobalsAA* invalidation might happen later in the pipeline than the
; optimization eliminating unnecessary loads/stores. 
; *Since GlobalsAA is a module-level analysis, any FunctionPass invalidating the GlobalsAA information will affect FunctionPass pipelines that execute later. For example, assume a FunctionPass1 | FunctionPass2 pipeline and 2 functions to be processed: f1 and f2. Assume furthermore that FunctionPass1 uses GlobalsAA info to do an optimization, and FunctionPass2 invalidates GlobalsAA. Assume the function passes run in the following order: FunctionPass1(f1), FunctionPass2(f1), FunctionPass1(f2), FunctionPass2(f2). Then FunctionPass1 will not be able to optimize f2, since GlobalsAA will have been invalidated in FuntionPass2(f1). To try and also test this scenario, there is an empty function before and after *
;  the function we're checking so that one of
; them will be processed by the whole set of FunctionPasses before @f.
; That will ensure *that if the invalidation happens, it happens* before the actual
; optimizations on @f start.

Other than that, I'm just wondering if there are any guarantees that the pipeline couldn't process the functions in the order @f, @bar, @bar2?

[a.elovikov]

• Make the comment more detailed.

[kristof.beyls]

LGTM, Thanks!

[a.elovikov]

I have to note that after http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170410/444886.html
[llvm] r300200 - Re-apply "[GVNHoist] Move GVNHoist to function simplification part of pipeline."
that test starts failing.

[kristof.beyls]

In D32015#727113, @a.elovikov wrote:

I have to note that after http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170410/444886.html
[llvm] r300200 - Re-apply "[GVNHoist] Move GVNHoist to function simplification part of pipeline."
that test starts failing.

Does this mean that the test (if it had already been committed), would have found that r300200 breaks retaining GlobalsAA info; or is something wrong with the test?
Should GVNHoist be marked as preserving GlobalsAA? Or does it break GlobalsAA?

[a.elovikov]

In D32015#727116, @kristof.beyls wrote:

In D32015#727113, @a.elovikov wrote:

I have to note that after http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170410/444886.html
[llvm] r300200 - Re-apply "[GVNHoist] Move GVNHoist to function simplification part of pipeline."
that test starts failing.

Does this mean that the test (if it had already been committed), would have found that r300200 breaks retaining GlobalsAA info; or is something wrong with the test?
Should GVNHoist be marked as preserving GlobalsAA? Or does it break GlobalsAA?

Yes, it would have found that. GVNHoist does not mark GlobalsAA as preserved. It most probably should but I'm not totally sure about that.

llvm/trunk/test/Transforms/PhaseOrdering/globalaa-retained.ll

 ; RUN: opt -O3 -S < %s | FileCheck %s
 target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
 target triple = "aarch64"
 
 @v = internal unnamed_addr global i32 0, align 4
 @p = common global i32* null, align 8
 
+
+; This test checks that a number of loads and stores are eliminated,
+; that can only be eliminated based on GlobalsAA information. As such,
+; it tests that GlobalsAA information is retained until the passes
+; that perform this optimization, and it protects against accidentally
+; dropping the GlobalsAA information earlier in the pipeline, which
+; has happened a few times.
+
+; GlobalsAA invalidation might happen later in the FunctionPassManager
+; pipeline than the optimization eliminating unnecessary loads/stores.
+; Since GlobalsAA is a module-level analysis, any FunctionPass
+; invalidating the GlobalsAA information will affect FunctionPass
+; pipelines that execute later. For example, assume a FunctionPass1 |
+; FunctionPass2 pipeline and 2 functions to be processed: f1 and f2.
+; Assume furthermore that FunctionPass1 uses GlobalsAA info to do an
+; optimization, and FunctionPass2 invalidates GlobalsAA. Assume the
+; function passes run in the following order: FunctionPass1(f1),
+; FunctionPass2(f1), FunctionPass1(f2), FunctionPass2(f2). Then
+; FunctionPass1 will not be able to optimize f2, since GlobalsAA will
+; have been invalidated in FuntionPass2(f1).
+
+; To try and also test this scenario, there is an empty function
+; before and after the function we're checking so that one of them
+; will be processed by the whole set of FunctionPasses before @f. That
+; will ensure that if the invalidation happens, it happens before the
+; actual optimizations on @f start.
+define void @bar() {
+entry:
+  ret void
+}
+
 ; Function Attrs: norecurse nounwind
 define void @f(i32 %n) {
 entry:
   %0 = load i32, i32* @v, align 4
   %inc = add nsw i32 %0, 1
   store i32 %inc, i32* @v, align 4
   %1 = load i32*, i32** @p, align 8
   store i32 %n, i32* %1, align 4
   %2 = load i32, i32* @v, align 4
   %inc1 = add nsw i32 %2, 1
   store i32 %inc1, i32* @v, align 4
   ret void
 }
 
-; check variable v is loaded only once after optimization, which should be
-; prove that globalsAA survives until the optimization that can use it to
-; optimize away the duplicate load/stores on variable v.
+; check variable v is loaded/stored only once after optimization,
+; which should be prove that globalsAA survives until the optimization
+; that can use it to optimize away the duplicate load/stores on
+; variable v.
 ; CHECK:     load i32, i32* @v, align 4
+; CHECK:     store i32 {{.*}}, i32* @v, align 4
 ; CHECK-NOT: load i32, i32* @v, align 4
+; CHECK-NOT:     store i32 {{.*}}, i32* @v, align 4
+
+; Same as @bar above, in case the functions are processed in reverse order.
+define void @bar2() {
+entry:
+  ret void
+}