[PATCH] D39758: CodeGen: Fix pointer info in SplitVecOp_EXTRACT_VECTOR_ELT

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1787
   return DAG.getExtLoad(ISD::EXTLOAD, dl, N->getValueType(0), Store, StackPtr,
-                        MachinePointerInfo(), EltVT);
+                        MachinePointerInfo(UndefValue::get(Type::getInt32PtrTy(
+                            *DAG.getContext(), PtrInfo.getAddrSpace()))),
----------------
yaxunl wrote:
> efriedma wrote:
> > yaxunl wrote:
> > > efriedma wrote:
> > > > You can't use "UndefValue" here; a non-null "Value*" has to be something alias analysis will understand properly.
> > > > 
> > > > If you want to actually get this right, you need to use MachinePointerInfo::getFixedStack().  (CreateStackTemporary doesn't explicitly return the FrameIndex, but you can extract it from the returned FrameIndexSDNode.)  Or I guess we could add some way to represent an unknown pointer in a specific address-space, but that's probably not any simpler.
> > > getFixedStack can only be used with frame index with a constant offset, but here the offset is not constant. Here we only need to carry the information about address space. Therefore undef with a specific address space. The backend won't perform any optimization based on that and only uses the address space information.
> > > getFixedStack can only be used with frame index with a constant offset, but here the offset is not constant.
> > 
> > Okay.  You might need some other PseudoSourceValue, then.
> > 
> > > Therefore undef with a specific address space.
> > 
> > undef means "this access doesn't alias anything".  See BasicAAResult::aliasCheck.
> > 
> > > The backend won't perform any optimization based on that
> > 
> > Among other things, we use MachinePointerInfo for scheduling... so if you get the info wrong, we could move the load before the store.
> There is no existing PseudoSourceValue suitable for this.
> 
> On the other hand, undef seems to be just the right representation. This is a temporary value on stack and it is used only here and not aliased to anything else.
> On the other hand, undef seems to be just the right representation. This is a temporary value on stack and it is used only here and not aliased to anything else.

No, undef is clearly wrong.  There is one very important aliasing relationship here: the scalar load aliases the vector store.  "undef" aliases nothing.  (I'm not sure the current MachineInstr::mayAlias will actually come to that conclusion in this case, but that could easily change in the future if we do more aggressive alias analysis with stack temporaries.)


https://reviews.llvm.org/D39758