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
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/
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