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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 8 inline comments as done.
chandlerc added a comment.

Thanks! Submitting with fixes for essentially all of this. There is one question below that I'm happy to continue discussing and if any of the fixes didn't match what you were expecting just lemme know!



================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:736
+      };
+      bool Devirt = any_of(CallHandles, IsDevirtualizedHandle);
+
----------------
mehdi_amini wrote:
> 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...
It doesn't look impossible to me? We have run the pass once at this point, and had handles held across it. Let me know if I'm missing something here though, happy to look for a better way to structure the code.


================
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.
----------------
mehdi_amini wrote:
> readonce? Do you mean readnone?
Er, readonly. I've added a sentence to clarify. We first deduce readonly, and then readnone.

No idea where 'readonce' came from. Fixed that.


================
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)
----------------
mehdi_amini wrote:
> If I follow correctly: this is the devirtualization that will trigger the first restart.
Yes, and commented.


================
Comment at: test/Other/cgscc-devirt-iteration.ll:82
+  %f2 = load void ()*, void ()** %f2ptr
+  call void %f2() readonly
+; BEFORE: call void %f2()
----------------
mehdi_amini wrote:
> And this is devirtualized during the second iteration, enabling the third one to deduce the readnone.
Correct, and again, comments added for a future reader.


================
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)
----------------
mehdi_amini wrote:
> Should you check that BEFORE contains the call to llvm.memcpy and not just devirtualize regularly to memcpy?
Not just before, but all of them. Added.


================
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
----------------
mehdi_amini wrote:
> The comment here suggested that this RUN line was useful outside of the repetition?
Good point. I've restored it.


https://reviews.llvm.org/D23114