[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:
> 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?
Yes, all the places it's lowered to false now in O0 (GlobalISel/IRTranslator.cpp, SelectionDAG/FastISel.cpp, SelectionDAG/SelectionDAGBuilder.cpp) could check if it can be folded to true first. It feels somewhat ugly to have to do that, though.

Of course, it's ugly that we have to do this last-chance lowering in 3 places in the first place. Same issue exists for llvm.objectsize too. Like efriedma suggested earlier, they should probably be both be lowered somewhere else...I'm not really sure where the best elsewhere to move it would be, though.


https://reviews.llvm.org/D4276