[PATCH] D93762: SCCP: Refactor SCCPSolver

[Sanne Wouda via Phabricator via llvm-commits]

sanwou01 added a comment.

This looks like a fine first step to separate SCCPSolver from SCCP. I agree with @fhahn that the SCCPSolver class should be improved by pulling out implementation details into (separate, hidden) classes, but this is probably best reviewed in separate patches. At least it looks like the public interface of SCCPSolver is already restricted to what is needed by SCCP.



================
Comment at: include/llvm/Transforms/Scalar/SCCP.h:30
 #include "llvm/Transforms/Utils/PredicateInfo.h"
+#include "llvm/Transforms/Utils/SCCPSolver.h"
 
----------------
Ideally, this include should not be needed.

Perhaps keep the definition of `struct AnalysisResultsForFn` in this file (as it is part of the `runIPSCCP` interface) and create a similar (but differently named) struct for the implementation detail of the `SCCPSolver`.


================
Comment at: include/llvm/Transforms/Utils/SCCPSolver.h:14-16
+#ifndef LLVM_TRANSFORMS_SCALAR_SCCP_SOLVER_H
+#define LLVM_TRANSFORMS_SCALAR_SCCP_SOLVER_H
+
----------------
Reflect the new location


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93762