[PATCH] D23798: [Instruction] Introduce a predicate mustOperandBeConstant()

[Sanjay Patel via llvm-commits]

spatel added inline comments.

================
Comment at: lib/IR/Verifier.cpp:3872-3877
@@ -3871,1 +3871,8 @@
 
+  if (auto *II = dyn_cast<IntrinsicInst>(IF))
+    for (unsigned OpIdx = 0; OpIdx < CS.getNumArgOperands(); ++OpIdx) {
+      auto MaybeReason = II->mustOperandBeConstant(OpIdx);
+      if (MaybeReason != None)
+        Assert(isa<ConstantInt>(CS.getArgOperand(OpIdx)), MaybeReason.getValue());
+    }
+  
----------------
spatel wrote:
> Now that we have a function dedicated to verifying the constant params, shouldn't the corresponding checks in the switch below this be removed?
> 
> Either way, I think we have 2 patches in 1 at this point, so it should be split when committing into:
> 1. Add IntrinsicInst::mustOperandBeConstant()
> 2. Add Instruction::mustOperandBeConstant()
Hmm...I'm not sure if I'm following the downcast argument. When we reach this code, we've verified that the function signature and name are ok.
1. Can we assert that this line:
```
  if (auto *II = dyn_cast<IntrinsicInst>(IF))
```
cannot fail? Ie, Assert(isa<IntrinsicInst>(IF)).

2. Should we also assert that
```
II.getIntrinsicID() == ID
```
after #1?

I think that would ensure that the subsequent switch const arg checks are redundant (and therefore could be removed), or do you see some other way that we might fall through?


Repository:
  rL LLVM

https://reviews.llvm.org/D23798