[PATCH] D23114: [PM] Introduce a devirtualization iteration layer for the new PM.

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 27 12:13:04 PST 2016


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

LGTM.



================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:637
+  // We have to explicitly define all the special member functions because MSVC
+  // refuses to generate them.
+  DevirtSCCRepeatedPass(const DevirtSCCRepeatedPass &Arg)
----------------
Wasn't it a limitation from MSVC2013 that is lifted now that we don't support it anymore?


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:703
+
+    for (int i = 0;; ++i) {
+      PreservedAnalyses PassPA = Pass.run(*C, AM, CG, UR);
----------------
What about something more explicit than `i`?   `IterationNum` or something like that.
(Two reasons: 1) this is a long loop body and 2) i is used in the body (and also there is another nested loop also using an index i)


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:733
+        // We now have a direct call where previously we had an indirect call,
+        // so iterate to process this devirtualization site.
+        return true;
----------------
We could have some debug logging about "Found a devirtualized call from CS->getParent()->getParent()->getName() to F->getName()".


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:736
+      };
+      bool Devirt = any_of(CallHandles, IsDevirtualizedHandle);
+
----------------
On the first iteration, it is weird to check all the value handles to find an impossible devirtualized call, but I don't have a better suggestion to reorganize the code that does not make it equally annoying somewhere else...


================
Comment at: test/Other/cgscc-devirt-iteration.ll:36
+; The @test2_* functions check that when we need multiple (in this case 2)
+; repeatitions to compute some state that is incrementally exposed with each
+; one, the limit on repeatitions is enforced. So we make progress with
----------------
`s/repeatitions/repetitions/` (same in many other places in this patch)


================
Comment at: test/Other/cgscc-devirt-iteration.ll:43
+; is to have one indirect call that can only be resolved when the entire SCC is
+; deduced as readonce, and mark that indirect call at the call site as readonce
+; to make that possible.
----------------
readonce? Do you mean readnone?


================
Comment at: test/Other/cgscc-devirt-iteration.ll:60
+  %f1 = load void (void ()**)*, void (void ()**)** %f1ptr
+  call void %f1(void ()** %ignore)
+; CHECK: call void @readnone_with_arg(void ()** %ignore)
----------------
If I follow correctly: this is the devirtualization that will trigger the first restart.


================
Comment at: test/Other/cgscc-devirt-iteration.ll:82
+  %f2 = load void ()*, void ()** %f2ptr
+  call void %f2() readonly
+; BEFORE: call void %f2()
----------------
And this is devirtualized during the second iteration, enabling the third one to deduce the readnone.


================
Comment at: test/Other/cgscc-devirt-iteration.ll:100
+; CHECK-NOT: Function Attrs
+; BEFORE: define void @test3(i8* %src, i8* %dest, i64 %size)
+; AFTER: define void @test3(i8* nocapture readonly %src, i8* nocapture %dest, i64 %size)
----------------
Should you check that BEFORE contains the call to llvm.memcpy and not just devirtualize regularly to memcpy?


================
Comment at: test/Other/cgscc-observe-devirt.ll:5
-; Also check that adding an extra CGSCC pass after the function update but
-; without requiring the outer manager to iterate doesn't break any invariant.
-; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(function-attrs,function(gvn),function-attrs)' -S < %s | FileCheck %s --check-prefix=AFTER2
----------------
The comment here suggested that this RUN line was useful outside of the repetition?


https://reviews.llvm.org/D23114





More information about the llvm-commits mailing list