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

Thu Nov 24 08:35:48 PST 2022

spatel added inline comments.

================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:762
+    case Instruction::And:
+    case Instruction::Or:
+      return Op1;
----------------
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.

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

https://reviews.llvm.org/D138542