[PATCH] D12988: [Bug 24848] Use range metadata to constant fold comparisons with constant values

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2131
@@ -2130,1 +2130,3 @@
 
+static ConstantRange GetCRFromRangeMetadata(Value *Val, uint32_t BitWidth) {
+  if (Instruction *I = dyn_cast<Instruction>(Val)) {
----------------
I'm still not very happy with the interface here -- I'd rather have this function do *just* the metadata parsing; and have the `dyn_cast<Instruction>(..)` logic in its caller.  I'd also rename this `GetConstantRangeFromMetadata` (and not repeat "Range" in the `CR` bit and `RangeMetadata`):

```
static ConstantRange GetConstantRangeFromMetadata(MDNode *RangeMD, unsigned BitWidth) {
  ...
}
```

and in its caller

```
  if (auto *I = dyn_cast<Instruction>(Val))
    if (auto *Ranges = I->getMetadata(LLVMContext::MD_range))
      LHS_CR.intersectWith(GetConstantRangeFromMetadata(Ranges, LHS_BitWidth));
```

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2137
@@ +2136,3 @@
+
+      ConstantRange CR = ConstantRange(BitWidth, false);
+      for (unsigned i = 0; i < NumRanges; ++i) {
----------------
Why not `ConstantRange CR(BitWidth, false);`?

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2140
@@ +2139,3 @@
+        ConstantInt *Lower =
+            mdconst::extract<ConstantInt>(Ranges->getOperand(2 * i + 0));
+        ConstantInt *Upper =
----------------
[Optional]
This is a matter of style, but I'd have written these as

```
auto *Low = ...
auto *High = ...
```

since the types are obvious, and `Low`, `High` are briefer.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2146
@@ +2145,3 @@
+
+        CR = CR.unionWith(Range);
+      }
----------------
[Optional]
Why not `CR.unionWith({Lower->getValue(), Upper->getValue()})`?  Alternatively, if that does not work, `CR.unionWith(ConstantRange(Lower->getValue(), Upper->getValue()))`?

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2394
@@ +2393,3 @@
+
+    uint32_t LHS_BitWidth = Q.DL.getTypeSizeInBits(LHS->getType());
+
----------------
Can the `Width` declared in line 2286 be used here?  I can't tell if it is out of scope easily from phabricator.

================
Comment at: test/Transforms/InstCombine/icmp-range.ll:57
@@ -56,1 +56,3 @@
 
+; Constant not in range, should return true
+define i1 @test_not_in_range(i32* nocapture readonly %arg) {
----------------
Please add a few more test cases.  At the very least, add 

 - one that does not constant fold
 - one that folds to false
 - one that has a multiple sub ranges in the `!range` metadata


http://reviews.llvm.org/D12988