[PATCH] D138542: [InstSimplify] Use dominate condtion to simplify instructions

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:762
+    case Instruction::And:
+    case Instruction::Or:
+      return Op1;
----------------
bcl5980 wrote:
> spatel wrote:
> > spatel wrote:
> > > spatel wrote:
> > > > bcl5980 wrote:
> > > > > spatel wrote:
> > > > > > bcl5980 wrote:
> > > > > > > I'm a little worry about the `and`/`or` part. Recursive call to isImpliedByDomCondition looks a little heavy.
> > > > > > > I return Op1 here is because Op1 can be constant value but not sure it has any side effect or not.
> > > > > > Did you run any compile-time-tracker experiments?
> > > > > > We could leave those out of the initial patch, add a TODO, and then put them back in a follow-up patch to reduce risk.
> > > > > Hi @spatel, can you help to run the and/or part to see the compile-time-tracker result? I have no privilege to test.
> > > > Sure - I created a branch from current main and applied this patch to it:
> > > > https://llvm-compile-time-tracker.com/compare.php?from=bf7f87e62c3c8016886dc722eeae9062624a73ca&to=89cd9e520cfa3f67291c8ab7c9addad058a2a05c&stat=instructions:u
> > > > (note: you may be able to request permission to run experiments like that by sending a note to @nikic - it is a great resource; thanks!)
> > > > 
> > > > There seems to be a slight increase in time, but it could also be in the noise. If you want to compare vs. a CVP solution, I think it would be interesting to know the result.
> > > Sorry - after re-reading the comment, I realize I didn't run the experiment that was requested. 
> > > 
> > > I'll add and/or in another commit.
> > https://llvm-compile-time-tracker.com/?config=NewPM-O3&stat=instructions%3Au&remote=rotateright
> > 
> > So there does seem to be a noticeable cost to handling more instructions. This again suggests we might do better by creating a CVP patch.
> It looks the if we don't propagate and/or the compile time is acceptable. 
> I'm sorry but I don't know how to add the similar code into CVP. 
> It looks for now CVP only detect constant/constant range based on ValueLatticeElement.
Another headache thing I find here is the pass sequence. SimplifyCFGPass will convert the test case to select at very early time. Even if we add the similar code into CVP it still can't detect the select.
Maybe we need to limit the dominate condition search to only when `MaxRecurse == RecursionLimit` . 
Recursive search dominate condition can't get any improvement.


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

https://reviews.llvm.org/D138542