[PATCH] D142803: [LogicCombine 1/?] Implement a general way to simplify logical operations.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LogicCombine.h:22
+private:
+  LogicCombiner *Helper;
+  Value *Val;
----------------
bcl5980 wrote:
> nikic wrote:
> > It's weird that LogicalOpNode has a reference back to LogicCombiner. Is this just for printing? Would it be better to pass it to the print method?
> I call print in operator "<<" . I can't pass the LogicCombiner into the override operator "<<". Do you have any ideal for that?
Right, this would not work with `operator<<`, one would have to call the print method. If `<<` is used a lot, then this makes sense.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:854-855
 
+    // For now, we share logical expression on basic block level.
+    LogicalOpsHelper Helper;
+
----------------
bcl5980 wrote:
> bcl5980 wrote:
> > spatel wrote:
> > > What does the "for now" mean? Will it change in the future? What limitation does this imply?
> > There are 2 limitations to be trade off. here The higher level the LogicalOpsHelper create, the more logical node cached, which means it can save more cpu timing. But it will maintain more leaf nodes. By default the max of leaf node is 8 , which is not enough for whole function I guess.
> > So I write the comment here to mention me we can do something here later. Like split the helper based on types to make the code more efficient, adjust the default value of max leaf node number, use APInt to support more bits.
> > 
> > The most headache thing for me is test the cpu overhead. @nikic , can you add my github fork to the llvm-compile-time-tracker.com? This serial patches need a lot of CPU overhead tests I think.
> My github fork link is: https://github.com/bcl5980/llvm-project
I've added your fork now. Sorry, I didn't notice this request before.


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

https://reviews.llvm.org/D142803