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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec 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()))),
----------------
efriedma wrote:
> 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.)
Scalar load from scratch is not possible as far as I understand.


https://reviews.llvm.org/D39758