[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:
> > 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)
> > 
> Except that that it really _doesn't_ evaluate to true. :)
> 
> The only thing that turns a call to llvm.is.constant into "true" is the constant folding code, which doesn't get run in O0.
> 
But why couldn't it evaluate to true at O0? Is there a valid reason why not? I mean, the intrinsic has to be lowered somehow at O0, so why not just evaluate it to true in cases like this?


================
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:
> > 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?
> This code cannot handle calls with a struct as an argument, it hits an assertion failure about not expecting a struct.
> 
> But it only ran in the first place when canConstantFoldCallTo returns true (see up a few lines at 1171), so it didn't actually come up before, because the other functions don't take structs as arguments.
Ah! I see. Thanks.


https://reviews.llvm.org/D4276