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

Mon Apr 13 05:52:47 PDT 2020

spatel added a comment.

In D68408#1976807 <https://reviews.llvm.org/D68408#1976807>, @lebedev.ri wrote:

> In D68408#1976790 <https://reviews.llvm.org/D68408#1976790>, @spatel wrote:
>
> > In D68408#1976039 <https://reviews.llvm.org/D68408#1976039>, @lebedev.ri wrote:
> >
> > > 1. add `Abs` IR instruction (i think not)
> >
> >
> > Is the answer the same for an intrinsic?
>
>
> Ack, i meant instruction/intrinsic interchangeably here.

We have grown more accepting of intrinsics (overflow, saturating, funnel, etc.) recently, so I'm not sure if the old arguments against an abs intrinsic still hold. Is there anything in particular about abs() that makes it different?

>> The more we see these kinds of problems, the more I wish we had created intrinsics for abs/smin/smax/umin/umax long ago. We keep trying to work around this limitation in IR, but it doesn't seem worth it. We have abs() functions in source and an abs node in codegen, so we're creating intermediate ops that don't exist on either side of IR.
> 
> Note that we similarly don't have saturating multiplication, overflowing/saturating left-shift.

Yes, there's no clear line that I know of. It's like IR canonicalization - we make the rules up as we go along and adapt the surrounding logic to work with the current format. I think we've overcome the limitation for this patch, so we don't need to gate this patch on a decision, but I think we still have the problem. The clearest sign is that -- within instcombine -- we have bailouts for otherwise accepted canonicalizations only if we "matchSelectPattern()".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68408