[PATCH] D16875: MemorySSA Optimizations: Patch 1 of N
David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 25 15:01:35 PST 2016
davidxl added inline comments.
================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:711
@@ +710,3 @@
+/// MaybeDef, with no potentially clobbering operations in between them.
+/// Where potentially clobbering ops are memory barriers, aliased stores, ...
+static bool canUseBeReorderedAboveDef(const Instruction *Use,
----------------
Unfinished sentence here.
================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:713
@@ +712,3 @@
+static bool canUseBeReorderedAboveDef(const Instruction *Use,
+ const Instruction *MaybeDef) {
+ // We only check for load-load clobbers; everything else is AA's problem. :)
----------------
nit: MaybeDef -> MayDef
================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:714
@@ +713,3 @@
+ const Instruction *MaybeDef) {
+ // We only check for load-load clobbers; everything else is AA's problem. :)
+ if (!isa<LoadInst>(MaybeDef) || !isa<LoadInst>(Use))
----------------
This needs some explanation --> can the alias query be moved here to make this function more general?
================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:724
@@ +723,3 @@
+
+ // ...But volatile operations can be reordered with non-volatile operations.
+ //
----------------
Move this comment up and combine with the other volatile related comment.
================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:744
@@ -713,1 +743,3 @@
+ return false;
return AA->getModRefInfo(DefMemoryInst, Loc) & MRI_Mod;
+ }
----------------
This does not look correct:
When canUseBeReorderedAboveDef returns false, it is not ok to unconditionally feed it into the AA query -- unless AA query also honors the ordering constraint (I have not checked).
================
Comment at: test/Transforms/Util/MemorySSA/atomic-clobber.ll:16
@@ -14,3 +15,3 @@
%2 = load i32, i32* %a, align 4
- %3 = add i32 %1, %2
- ret i32 %3
+ ret void
+}
----------------
what is this change about?
================
Comment at: test/Transforms/Util/MemorySSA/atomic-clobber.ll:41
@@ +40,3 @@
+ %1 = load atomic i32, i32* %a acquire, align 4
+; CHECK: MemoryUse(1)
+; CHECK-NEXT: %2 = load atomic i32
----------------
Is this correct? The atomic load can not be reordered before the previous load with acquire.
================
Comment at: test/Transforms/Util/MemorySSA/atomic-clobber.ll:44
@@ +43,3 @@
+ %2 = load atomic i32, i32* %a unordered, align 4
+; CHECK: 2 = MemoryDef(1)
+; CHECK-NEXT: %3 = load atomic i32
----------------
Is this too conservative?
http://reviews.llvm.org/D16875
More information about the llvm-commits
mailing list