[PATCH] D60377: [ConstantRange] Add PreferredResultType support for unionWith()

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 4 inline comments as done.
nikic added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:470
 
+static ConstantRange getPreferredRange(
+    const ConstantRange &CR1, const ConstantRange &CR2,
----------------
lebedev.ri wrote:
> Please do this as preparatory NFC commit.
I've included this change while committing D59959.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:609
   if (!isUpperWrapped() && !CR.isUpperWrapped()) {
-    if (CR.Upper.ult(Lower) || Upper.ult(CR.Lower)) {
-      // If the two ranges are disjoint, find the smaller gap and bridge it.
-      APInt d1 = CR.Lower - Upper, d2 = Lower - CR.Upper;
-      if (d1.ult(d2))
-        return ConstantRange(Lower, CR.Upper);
-      return ConstantRange(CR.Lower, Upper);
-    }
+    if (CR.Upper.ult(Lower) || Upper.ult(CR.Lower))
+      return getPreferredRange(
----------------
lebedev.ri wrote:
> And here we have
> ```
>     //  L---U            : this
>     //        L---U      : CR
> or
>     //        L---U      : this
>     //  L---U            : CR
> 
> =>
> 
>     //  L---------U      : this
> or
>     //        L--------- : this
>     // -----U            : CR
> ```
> ?
Right. I've added a comment.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:635
     //       L---U       : CR
-    //    <d1>  <d2>
-    if (Upper.ule(CR.Lower) && CR.Upper.ule(Lower)) {
-      APInt d1 = CR.Lower - Upper, d2 = Lower - CR.Upper;
-      if (d1.ult(d2))
-        return ConstantRange(Lower, CR.Upper);
-      return ConstantRange(CR.Lower, Upper);
-    }
+    if (Upper.ult(CR.Lower) && CR.Upper.ult(Lower))
+      return getPreferredRange(
----------------
lebedev.ri wrote:
> So we will produce 
> ```
>     // ----U       L---- : this
>     //       L---U       : CR
> =>
>     // ----------U L----
> or
>     // ----U L----------
> ```
> and in either case we bridge either of the gaps?
Right. I've added the two possible results to the existing comment.


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:367
+template<typename Fn1, typename Fn2>
+void testBinarySetOperationExhaustive(Fn1 OpFn, Fn2 InResultFn) {
   unsigned Bits = 4;
----------------
lebedev.ri wrote:
> Also, could you please do this as NFC commit
Done in rL357874.


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

https://reviews.llvm.org/D60377