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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
