[PATCH] D68408: [InstCombine] Negator - sink sinkable negations

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:180
+  }
+  case Instruction::SDiv:
+    // `sdiv` is negatible if divisor is not undef/INT_MIN/1.
----------------
lebedev.ri wrote:
> spatel wrote:
> > lebedev.ri wrote:
> > > nikic wrote:
> > > > Noticed while looking through the tramp3d-v4 diff: This should be behind a one-use check, to avoid duplicating expensive division instructions.
> > > I was hesitant about this one indeed, it isn't a typo that one-use check has gone away here,
> > > because we //generally// consider only instruction count.
> > > 
> > > @spatel thoughts?
> > > 
> > > But the main, bigger question this touches is:
> > > "but what if all the uses would get negated by us?
> > > In future, can we somehow sanely model the whole negatible tree,
> > > not giving up at non-single-use instructions,
> > > but defer that to after we've finished building new tree?"
> > > 
> > > 
> > I missed that logic difference, and I'm not getting a reviewable diff of the attached files with llvm-diff or other apps. Can you create an IR example/regression test for that?
> @spatel
> To be clear, the "bigger question" is pretty rhetorical, or at least not for this review.
> 
> The actual question here is whether we should consider `sdiv` special,
> and even if we can negate it without increasing instruction count,
> we should only do so if there are no other uses of old `sdiv`.
> 
There is precedence for this kind of special treatment. In other words, not all opcodes are equal in terms of analysis (and secondary concern of codegen), and we will even increase instruction count to avoid some like div/rem (although those transforms are probably currently not safe with respect to poison).

If it would be NFC-ish to keep the one-use check, then we should do that. Then, remove the limitation as a follow-up if that can be shown useful?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68408