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

[David Majnemer via llvm-commits]

majnemer added a comment.

There are more specific constraints for some intrinsics.  For example, stackprotector must not have a phi or select as its operand: it must be an `AllocaInst`.

I wonder if the right thing to do here is have a different function (somewhere) which determines if an `llvm::Value *` is an appropriate operand for some `llvm::User *`.


================
Comment at: lib/IR/Instruction.cpp:499-500
@@ +498,4 @@
+  case Instruction::Call:
+  case Instruction::Invoke:
+    if (auto *I = dyn_cast<IntrinsicInst>(this))
+      return I->mustOperandBeConstant(OpIdx) != None;
----------------
This will not work, Invokes are not descendants of IntrinsicInst.

================
Comment at: lib/IR/IntrinsicInst.cpp:88-89
@@ +87,4 @@
+Optional<const char*>
+IntrinsicInst::mustOperandBeConstant(unsigned OpIdx) const {
+  switch (getIntrinsicID()) {
+  default:
----------------
This seems incomplete (it is missing at least prefetch and coro_id).

================
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:
> 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?
An IntrinsicInst is a descendant of CallInst but the CallSite might be an InvokeInst of an intrinsic function.  IIUC, this would make it possible for the dyn_cast to fail.


Repository:
  rL LLVM

https://reviews.llvm.org/D23798