[PATCH] D84874: [SVE][CodeGen] Legalisation of INSERT_VECTOR_ELT for scalable vectors

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1510
+  StackPtr = DAG.getMemBasePlusOffset(StackPtr, IncrementSize, dl,
+                                      N->getFlags(), LoVT.isScalableVector());
 
----------------
efriedma wrote:
> kmclaughlin wrote:
> > efriedma wrote:
> > > Can we pass a TypeSize here, instead of passing the offset and scalable bit separately?
> > Since TypeSize uses uint64_t, I thought it would be better to pass in IsScalable as a separate argument so that we can keep the Offset argument as a signed value.
> We could introduce a dedicated TypeOffset type if you think that's really an issue.  I suspect getMemBasePlusOffset is rarely, if ever, used with a negative offset, though.  Maybe add an overload that takes a TypeSize and go from there.
> 
> I really want to avoid getKnownMinSize() where it isn't necessary; it's going to be a lot easier to make mistakes passing around possibly-scaled offsets as bare integers.
I had a look through the uses of getMemBasePlusOffset (and getObjectPtrOffset) and I think you're right that it is not used with negative offsets, so I created a separate patch which changes the type of the Offset argument rather than adding an overload


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84874/new/

https://reviews.llvm.org/D84874