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

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl 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()))),
----------------
rampitec wrote:
> rampitec wrote:
> > yaxunl wrote:
> > > rampitec wrote:
> > > > 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.
> > > How about introduce a new PseudoSourceValue kind "AddressSpace". The only known information about this pointer is its address space.
> > To me it is an Undef in an obscure package. I think undef is just fine here. If we would use it for anything the a PseudoSourceValue holding a FI is a right choice.
> Also, I would say it does not make situation worse than before the change. Before it was just dummy MachinePointerInfo(). I do not really think all problems shall be solved in a single patch.
I found a way to represent a stack memory with unknown offset and have updated the patch. Please take a look. Thanks.


https://reviews.llvm.org/D39758