[PATCH] D4276: Added llvm.is.constant intrinsic

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/docs/LangRef.rst:14432-14433
+The result of the intrinsic will depend on what optimisations are
+run. In particular, note that it will always be false if constant
+folding is not done.
+
----------------
void wrote:
> This is true only for expressions that evaluate to constants not immediates, right?
> 
> Should you also mention that if a function with llvm.is.constant() is inlined, any constants fed into the inlined function may cause llvm.is.constant() to return true? (or something to that affect)
If you build with -O0, constant folding is not done, so even the @test_constant() function will return false.

(Of course, at the C level, the __builtin_constant_p intrinsic will still return true at -O0 when given a frontend constant).

Updated the docs to mention inlining explicitly.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:838
+// Intrinsic to detect whether its argument is a constant.
+def int_is_constant : Intrinsic<[llvm_i1_ty], [llvm_any_ty], [IntrNoMem], "llvm.is.constant">;
+
----------------
void wrote:
> 80-columns?
Fixed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5762
+
+    case Intrinsic::is_constant:
+    // If this wasn't constant-folded away by now, then it's not a
----------------
efriedma wrote:
> Indentation.
Fixed.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1176
            AI != E; ++AI) {
+        if (AI->get()->getType()->isStructTy())
+          return markOverdefined(I); // Can't handle struct args.
----------------
void wrote:
> This seems unrelated. Was it meant to be in this patch? If so, could you comment that it's specifically related to llvm.is.constant()?
It's due to llvm.is.constant being able to take a struct type as an argument. This code gets confused by seeing a call for which is canConstantFoldCallTo() returns true, but which can take a struct as an argument.

I'll add a bit to the commit message remarking why the change was made, but I don't think the code here really need mention llvm.is.constant.


https://reviews.llvm.org/D4276