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

[Bill Wendling via Phabricator via llvm-commits]

void 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.
+
----------------
jyknight wrote:
> 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.
My comment is for documenting the llvm.is.constant intrinsic, not __builtin_constant_p(). :-) (People can write their own .ll files.) I just wanted to make sure that people know that this will evaluate to "true" even without constant folding:

  %0 = i1 llvm.is.constant.i32(i32 37)



================
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.
----------------
jyknight wrote:
> 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.
I'm still a bit confused. This change will affect all call sites, not just those to llvm.is.constant. Or are you saying that the only time this situation will come up is when it involves the llvm.is.constant call?


https://reviews.llvm.org/D4276