[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Nicolai Hähnle via Phabricator via llvm-commits]

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