InstCombine Question

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

InstCombine Question

dag-7
I am confused by this bit of code in instcombine:

09789   if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(Op)) {
09790     const Value *GEPI0 = GEPI->getOperand(0);
09791     // TODO: Consider a target hook for valid address spaces for this
xform.
09792     if (isa<ConstantPointerNull>(GEPI0) &&
09793         cast<PointerType>(GEPI0->getType())->getAddressSpace() == 0) {
09794       // Insert a new store to null instruction before the load to
indicate
09795       // that this code is not reachable.  We do this instead of
inserting
09796       // an unreachable instruction directly because we cannot modify
the
09797       // CFG.
09798       new StoreInst(UndefValue::get(LI.getType()),
09799                     Constant::getNullValue(Op->getType()), &LI);
09800       return ReplaceInstUsesWith(LI, UndefValue::get(LI.getType()));
09801     }
09802   }

First, what happens to the StoreInst?  It looks like it is not attached
anywhere.

Second, I am seeing this happen in a block that is definitely reachable.
Later on the null GEP is removed because it isInstructionTriviallyDead.
But the undef store to null remains since the block is in fact reachable.
This is obviously bad.

So how does the undef store to null appear in the IR when it isn't attached
anywhere and how can I get rid of it?

What is this code trying to do, anyway?

                                                 -Dave
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: InstCombine Question

Chris Lattner
On Fri, 4 Apr 2008, David Greene wrote:

> I am confused by this bit of code in instcombine:
>
> 09789   if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(Op)) {
> 09790     const Value *GEPI0 = GEPI->getOperand(0);
> 09791     // TODO: Consider a target hook for valid address spaces for this
> xform.
> 09792     if (isa<ConstantPointerNull>(GEPI0) &&
> 09793         cast<PointerType>(GEPI0->getType())->getAddressSpace() == 0) {
> 09794       // Insert a new store to null instruction before the load to
> indicate
> 09795       // that this code is not reachable.  We do this instead of
> inserting
> 09796       // an unreachable instruction directly because we cannot modify
> the
> 09797       // CFG.
> 09798       new StoreInst(UndefValue::get(LI.getType()),
> 09799                     Constant::getNullValue(Op->getType()), &LI);
> 09800       return ReplaceInstUsesWith(LI, UndefValue::get(LI.getType()));
> 09801     }
> 09802   }
>
> First, what happens to the StoreInst?  It looks like it is not attached
> anywhere.

The &LI argument causes it to be inserted before the load.

> Second, I am seeing this happen in a block that is definitely reachable.
> Later on the null GEP is removed because it isInstructionTriviallyDead.
> But the undef store to null remains since the block is in fact reachable.
> This is obviously bad.

This xform doesn't have anything to do with block reachability.  It
effectively transforms "load from null pointer" into an unreachable
instruction.  Load from null pointer is undefined, so this is a safe xform
regardless of where it occurs.

>
> So how does the undef store to null appear in the IR when it isn't attached
> anywhere and how can I get rid of it?

Don't do undefined behavior? :)

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: InstCombine Question

dag-7
On Friday 04 April 2008 13:07, Chris Lattner wrote:

> > So how does the undef store to null appear in the IR when it isn't
> > attached anywhere and how can I get rid of it?
>
> Don't do undefined behavior? :)

I don't think it's undefined behavior.  Right before instcombine, we have
this:

        %r60 = load <2 x i64>* %"$LCS_1", align 16 ; <<2 x i64>> [#uses=2] ;  
srcLine 41
        %r61 = extractelement <2 x i64> %r60, i32 0 ; <i64> [#uses=1] ;  srcLine 41
        %r62 = getelementptr <2 x double>* null, i32 0, i64 %r61 ; <double*>
[#uses=1] ;  srcLine 41
        %r63 = load double* %r62 ; <double> [#uses=1] ;  srcLine 41

So we're loading a vector of pointers and then using getelementptr as
basically a reg-reg copy with a cast (I think).  Yes, it's a little weird, but
legal  AFAICS.

I'm not sure if the above code is really kosher.  It is very strange to see
the typing of the extractelement and GEP here.

But instcombine only looks at the first operand to GEP, sees it's null and
just assumes it's useless code.  Not the case in this example.

So I think this is actually a bug in instcombine and maybe elsewhere as
well.  What  do you think?

                                                -Dave
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: InstCombine Question

dag-7
On Friday 04 April 2008 13:42, David Greene wrote:

> On Friday 04 April 2008 13:07, Chris Lattner wrote:
> > > So how does the undef store to null appear in the IR when it isn't
> > > attached anywhere and how can I get rid of it?
> >
> > Don't do undefined behavior? :)
>
> I don't think it's undefined behavior.  Right before instcombine, we have
> this:
>
> %r60 = load <2 x i64>* %"$LCS_1", align 16 ; <<2 x i64>> [#uses=2] ;
> srcLine 41
> %r61 = extractelement <2 x i64> %r60, i32 0 ; <i64> [#uses=1] ;  srcLine
> 41 %r62 = getelementptr <2 x double>* null, i32 0, i64 %r61 ; <double*>
> [#uses=1] ;  srcLine 41
> %r63 = load double* %r62 ; <double> [#uses=1] ;  srcLine 41
>
> So we're loading a vector of pointers and then using getelementptr as
> basically a reg-reg copy with a cast (I think).  Yes, it's a little weird,
> but legal  AFAICS.
>
> I'm not sure if the above code is really kosher.  It is very strange to see
> the typing of the extractelement and GEP here.

Actually, there are multiple problems.  The first zero index on the GEP is
wrong.  I believe that it should be the following:

   %r62 = getelementptr <2 x double>* null, i64 %r61

This is essentially the equivalent of a bitcast, then.

The second problem is the typing of the GEP and the following load.
Clearly that's wrong.  The GEP should return a double *.

These are problems in our compiler, not LLVM.  But I do want to ask
about the legality of the revised GEP above.  Is that legal code?  If so,
instcombine should not be removing it as dead code.

                                                     -Dave
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: InstCombine Question

Chris Lattner
On Fri, 4 Apr 2008, David Greene wrote:

>> So we're loading a vector of pointers and then using getelementptr as
>> basically a reg-reg copy with a cast (I think).  Yes, it's a little weird,
>> but legal  AFAICS.
>>
>> I'm not sure if the above code is really kosher.  It is very strange to see
>> the typing of the extractelement and GEP here.
>
> Actually, there are multiple problems.  The first zero index on the GEP is
> wrong.  I believe that it should be the following:
>
>   %r62 = getelementptr <2 x double>* null, i64 %r61
>
> This is essentially the equivalent of a bitcast, then.

Right.  I guess it would be acceptable to turn into into an inttoptr
instruction.

> These are problems in our compiler, not LLVM.  But I do want to ask
> about the legality of the revised GEP above.  Is that legal code?  If so,
> instcombine should not be removing it as dead code.

I don't think it's really legal, GEP shouldn't be used that way.

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev