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

[Chris Lattner via Phabricator via cfe-commits]

lattner requested changes to this revision.
lattner added a comment.
This revision now requires changes to proceed.

This looks like a great direction, but please make sure to minimize public implementation details.  We don't want the vast majority of instcombine to be visible outside of its library (it is hairy enough as it is :-)



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:29
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Support/KnownBits.h"
 #include <functional>
----------------
Can this be forward declared instead of #include'd?


================
Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:30
+#include "llvm/Transforms/Utils/Local.h"
+#include <cassert>
+
----------------
Please minimize #includes in general, thanks :)


================
Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:46
+/// combine them.
+class LLVM_LIBRARY_VISIBILITY InstCombiner {
+public:
----------------
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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81728