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

Sanjay Patel via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 23 08:52:16 PDT 2016


spatel added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1383-1385
@@ -1382,5 @@
-                                          unsigned OpIdx) {
-  // Early exit.
-  if (!isa<Constant>(I->getOperand(OpIdx)))
-    return true;
-
----------------
jmolloy wrote:
> spatel wrote:
> > Is this check removed intentionally? Ie, we must assume that the instruction may be malformed?
> I removed it because I felt that having the predicate in Instruction provide possibly different results based on the current operands would be confusing to debug.
> 
> Thinking about it though, having a predicate say "this operand must be constant" when it is currently a variable (because the predicate is being conservative) probably isn't ideal.
> 
> I'm conflicted about the right way to go here.
The difference comes into play with intrinsics. And that's one of the motivating cases in SimplifyCFG, right?

Without the extra check, we'll say that all intrinsics require all constant argument operands, and so we won't be able to sink them using phis. If we add the check and see that the intrinsic operand is already a variable, then we may still be able to do the sinking transform on the intrinsic.

In order to keep this patch 'NFC', I'd keep the check. If we decide to change the behavior, I think it should be another patch.

Side note on intrinsic operand requirements: it seems the only place that a constant requirement is encoded/enforced is in the Verifier. Ie, it's not part of the td def itself. If that's right, that part of the Verifier should be refactored, so we can use it here and have a single point of truth for intrinsic operand const-ness.


Repository:
  rL LLVM

https://reviews.llvm.org/D23798





More information about the llvm-commits mailing list