[PATCH] D69387: [ConstantRange] Add toKnownBits() method

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

So, this looks fine, but I'm still not quite clear on the use-case. I thought this might be useful for computing ranges of bit ops, but now that I see the implementation, I don't think that's the case. We just get the known top bits, but lose any information about the low bits (which can still be used, though in an operation-specific manner).

We should probably have some case where we can actually use this before landing.



================
Comment at: llvm/lib/IR/ConstantRange.cpp:117
+  Known.One.clearLowBits(*Bit);
+  Known.Zero.clearLowBits(1 + *Bit);
+
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > nikic wrote:
> > > Why is it necessary to clear one more bit in the Zero case?
> > Technically, it's the other way around, we can/need to clean exactly `1 + *Bit` low bits from them,
> > but in the case of `Known.One`, the `1 + *Bit` bit is already always unset (because i used `getUnsignedMin()`),
> > so it doesn't require unsetting.
> > 
> To spell this out further:
> `HighestTransientBit` is the most significant bit that is different between `getUnsignedMin()` and `getUnsignedMax()`.
> All bits (if any) that are higher/more significant/more MSB are all identical.
> Which means, since `getUnsignedMin()` (which is less than `getUnsignedMax()`) is used as baseline reference
> for `Known.One` and `Known.Zero`, the `HighestTransientBit`'th bit is *not* One, it is Zero.
> So we only need to unset the Zero knowledge about it. But sure, we could unset both the One and Zero knowledge,
> but that won't ever matter since this algo already results in precise match.
> 
> Let me know if that did/didn't explain it.
I understand the reasoning, but would suggest to just clear `1 + HighestTransientBit` in both cases -- apart from degenerate cases it doesn't matter for performance, and it saves having to explain it in comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:92
+
+  Known.One = getUnsignedMin();
+  Known.Zero = ~getUnsignedMin();
----------------
getUnsignedMin() is used three times and not free. Store in var?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69387