[PATCH] D86928: [SVE][CodeGen] Fix TypeSize/ElementCount related warnings in sve-split-store.ll

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 14 12:06:18 PDT 2020


efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:544
+  TypeSize StWidth = StVT.getSizeInBits();
+  TypeSize StSize = StVT.getStoreSizeInBits();
   auto &DL = DAG.getDataLayout();
----------------
david-arm wrote:
> efriedma wrote:
> > I don't think it's possible for StVT to be a vector type here.
> I think the final else case covers vectors, right? We can have truncating vector stores. With the way the code is structured we test for the scalar cases first and therefore we were hitting the TypeSize->uint64_t cast warnings before we got to the final else case. That's why I converted the code to using TypeSize here. Alternatively, if you want I can restructure the code along the following lines:
> 
> if (StVT.isVector) {
>   ...
>   return ...;
> }
> 
> unsigned StWidth = StVT.getSizeInBits().getFixedSize();
> unsigned StSize = StVT.getStoreSizeInBits().getFixedSize();
> ... deal with scalar cases ...
> 
We can't have illegal truncating vector stores here; they have to be handled by LegalizeVectorOps.

I guess because of the way the code is structured, we don't check whether the store is legal until later, so we can have a legal truncating vector store here?  Which is weird, but I guess you don't need to restructure the code in this patch.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:557
     ReplaceNode(SDValue(Node, 0), Result);
-  } else if (StWidth & (StWidth - 1)) {
+  } else if (!isPowerOf2_64(StWidth.getKnownMinSize())) {
     // If not storing a power-of-2 number of bits, expand as two stores.
----------------
sdesmalen wrote:
> david-arm wrote:
> > sdesmalen wrote:
> > > You're using `getKnownMinSize()` here but using `getFixedSize()` below on line 560. Is this missing a condition, e.g.:
> > > ```!StVT.isScalableVector() && !isPowerOf2_64(StWidth.getFixedSize())```
> > > or perhaps (because it seems like this only applies to integers):
> > > ```!StVT.isVector() && !isPowerOf2_64(StWidth.getFixedSize())```
> > Yes, I used the known min size here because it could be a scalable vector (since we haven't hit the final else case yet). The equivalent load function has the same problem (see https://reviews.llvm.org/D86697 that was checked in recently). So we either:
> > 
> > 1. Restructure the code so that the vector case comes first.
> > 2. Use getKnownMinSize.
> > 3. Use !StVT.isVector() && !isPowerOf2_64(StWidth.getFixedSize())
> > 
> > I'm happy to go with either way - whichever you and @efriedma prefer really.
> 3 would have my preference.
3 is fine.  Ideally, we'd refactor the code, I guess, but better to keep that separate.

(Theoretically, it's also possible to have a legal truncstore with a three-element vector, although no in-tree target does that.  Since we're messing with the logic anyway, might as well handle that correctly.)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2455
     MMO = DAG.getMachineFunction().getMachineMemOperand(
-        N->getPointerInfo().getWithOffset(HiOffset), MachineMemOperand::MOStore,
-        HiMemVT.getStoreSize(), Alignment, N->getAAInfo(), N->getRanges());
+        MPI, MachineMemOperand::MOStore, HiSize, Alignment, N->getAAInfo(),
+        N->getRanges());
----------------
Do we need to fix Alignment here?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2235
+  if (V.getValueType().isScalableVector())
+    return SDValue();
+
----------------
efriedma wrote:
> I don't think we should ever get here with a scalable vector?  We should guard the creation of DemandedElts.
Is `if (!DemandedElts)` related to this patch somehow?


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

https://reviews.llvm.org/D86928



More information about the llvm-commits mailing list