[PATCH] D70043: [ConstantRange] Add `mulWithNoWrap()` method

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

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

> In D70043#1771089 <https://reviews.llvm.org/D70043#1771089>, @nikic wrote:
>
> > > Baseline ConstantRange::mul() does not have an exhaustive test,
> > >  and if i add one (TestUnsignedBinOpExhaustive()+/*CorrectnessOnly=*/true),
> > >  it doesn't pass.
> >
> > For my own sanity: This doesn't mean that the multiply() implementation is incorrect,
> >  just that this testing function is a bit overly aggressive in CorrectnessOnly mode.
> >  It expects the result to be a superset of the minimal unsigned bounding range.
>
>
> Sure, of course, otherwise the `EXPECT_TRUE(CR.contains(N));` would complain.
>
> > We might want to change the function to actually only check correctness via `EXPECT_TRUE(CR.contains(N))`.
>
> Note that we already do that check, otherwise we wouldn't know that the result is at least conservatively correct.
>
> > ...
>
>
>
> > So I guess it would be pretty hard to make this work non-approximately...


As i have previously said, what's *really* missing is QoI testing:
just measure how constrained is the ConstantRange-returned result,
as compared to the ideal result, with `<1` being not conservatively correct,
`=1` ideal, and `>1` non-optimal. That can totally replace
the existing `EXPECT_EQ(Exact, CR);` check,
and be **that** much more informative.

@reames @nikic But in the end, any suggestions on how to get *these* patches moving?
The replies to the maillist post are non-conclusive.
Anyone else have an opinion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70043