[PATCH] D28137: [Devirtualization] MemDep returns non-local !invariant.group dependencies

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:911
   Result.clear();
-
+  {
+    auto NonLocalDef = NonLocalDefsCache.find(QueryInst);
----------------
Prazek wrote:
> davide wrote:
> > why do you need a different scope?
> This function is pretty large, so I prefer not to add new names with that huge scope.
I agree it's large, maybe in a separate commit you can try splitting into multiple functions?
Not sure how much is buying us as MemDep will go away soon'ish (6 months to 1 year)  but it's worth a try.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:912
+  {
+    auto NonLocalDef = NonLocalDefsCache.find(QueryInst);
+    if (NonLocalDef != NonLocalDefsCache.end()) {
----------------
Prazek wrote:
> davide wrote:
> > The type of `NonLocalDef`  is not entirely obvious so I'd prefer if you use the explicit one.
> Find returns iterator, so I would like to avoid this long name. Does adding "It" suffix to variable name works for you?
Fair enough.


================
Comment at: test/Transforms/NewGVN/invariant.group.ll:345-347
+; CHECK-LABEL: define void @handling_loops()
+define void @handling_loops() {
+  %a = alloca %struct.A, align 8
----------------
Prazek wrote:
> davide wrote:
> > Thanks for adding NewGVN tests (although they're still XFAIL'ed, alas).
> BTW is there any way we could merge them together keeping the Xfail semantics?
I wish there was (as it would avoid a lot of duplication) but I wasn't able to find it. Happy to do the conversion if you think of/implement a way to share.

Side note: I think this is not really high priority as the only time when we copy tests is during transitions (as in pass rewrites etc...) which are not common enough to justify the burden of implementing a conditional XFAIL on a per-pass basis. YMMV.


https://reviews.llvm.org/D28137