[PATCH] D47702: DAG: ComputeNumSignBits from load range metadata

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3536-3537
+      ConstantRange CR = getConstantRangeFromMetadata(*Ranges);
+      return std::min(CR.getLower().getNumSignBits(),
+                      CR.getUpper().getNumSignBits());
+    }
----------------
nhaehnle wrote:
> First, this should probably use getSignedMin() and getSignedMax() instead of getLower() and getUpper(). An interesting test for this would be a range `{i32 -1, i32 1}`, which to my understanding only allows the values -1 and 0, so should have 32 sign bits, while the code as-is would say that it has 31 sign bits.
> 
> The other thing that worries me is what happens when there is a mismatch between the type used in the definition of the range metadata and the type returned by the SDNode. At the very least, there needs to be an assert to check against that case if we think it can't happen. If it //can// happen, it needs to be handled by taking the bit widths of the type of `LD` and of the APInts in the constant range into account.
I just double-checked, and at least the ConstantRange always has the same bit width for lower and upper bounds. But we still need to guard against that bit width being different from the bit width of the SDNode.


https://reviews.llvm.org/D47702