nhaehnle added inline comments.
================
Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:46
+/// combine them.
+class LLVM_LIBRARY_VISIBILITY InstCombiner {
+public:
----------------
lattner wrote:
> I would really rather not make this be a public class - this is a very thick
> interface. Can this be cut down to something much smaller than the
> implementation details of InstCombine?
>
> If you're curious for a pattern that could be followed, the MLIR AsmParser is
> a reasonable example. The parser is spread across a bunch of classes in the
> lib/ directory:
> https://github.com/llvm/llvm-project/blob/master/mlir/lib/Parser/Parser.cpp
>
> But then there is a much smaller public API exposed through a header:
> https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/OpImplementation.h#L229
>
>
I agree with the sentiment, but note @Flakebi has split up the `InstCombiner`
class into `InstCombiner` and `InstCombinerImpl` classes, which addresses those
concerns already as far as I'm concerned.
Looking through the new `InstCombiner`, aside from methods that are core to the
workings of InstCombine (modifying instructions while keeping track of the
Worklist) and methods for accessing the analyses, what's left is:
* A bunch of static methods that should arguably just be global functions in a
utils header somewhere.
* CreateOverflowTuple and CreateNonTerminatorUnreachable
Moving those methods feels sensible, but is likely to touch a lot of code, so I
think it would be better to do it in a separate commit.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81728/new/
https://reviews.llvm.org/D81728
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
