[PATCH] D93838: [SCCP] Add Function Specialization pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D93838#2807597 <https://reviews.llvm.org/D93838#2807597>, @SjoerdMeijer wrote:

> In D93838#2807515 <https://reviews.llvm.org/D93838#2807515>, @fhahn wrote:
>
>> In D93838#2793238 <https://reviews.llvm.org/D93838#2793238>, @SjoerdMeijer wrote:
>>
>>> Addressed @fhahn 's comments: don't run the solver for specialised functions  removed the recursive specialization test for now.
>>
>> I'm not sure if removing the recursive specialization test is the best thing to do, without known what it is supposed to test? If it is a legitimate test, I thin it would be good to keep it.
>
> Thanks for the comments. Since they are minor, I will fix it before committing. I.e., will remove that update to solver and add the test back.

There are at least a few more places that may modify the solver state after the solver is run. I think it would be good to audit them and update the patch. IIUC in the current version there should be no need to update the solver state after `RunSolver`. It would also be good to also check if there any places with updates I missed.



================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:146
+        if (Solver.isBlockExecutable(I->getParent()))
+          Solver.visit(I);
+      }
----------------
This should also not be needed?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:154
+        I->eraseFromParent();
+        Solver.removeLatticeValueFor(I);
+      }
----------------
also not needed?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:233
+        // with the given value.
+        Solver.markArgInFuncSpecialization(F, ClonedArg, C);
+
----------------
Is this needed if the solver does not run again?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:243
+      if (!IsPartial)
+        Solver.markFunctionUnreachable(F);
+
----------------
Is this needed if the solver does not run again?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93838/new/

https://reviews.llvm.org/D93838