[PATCH] D84937: [SVE][CodeGen] Fix scalable vector issues in DAGTypeLegalizer::GenWidenVectorStores

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:5157
+      Type *NewTy = NewVT.getTypeForEVT(*DAG.getContext());
+      NewAlign = DAG.getDataLayout().getPrefTypeAlign(NewTy);
+    } else
----------------
efriedma wrote:
> efriedma wrote:
> > getPrefTypeAlign() isn't related to anything relevant.  We need to compute the common alignment based on the original alignment, the offset of the original MachinePointerInfo, and the current offset.
> Something like `commonAlignment(ST->getAlign(), Idx * ValEltWIdth)`, I guess?  Maybe some better way to compute the offset so far from the loop.
OK, I can try that, but I'm not sure what to do with the offset? The loop currently (in theory) permits arbitrary offsets containing any combination of scalable vectors, fixed vectors and scalar types. I could re-introduce the Offset variable that I tried to eliminate from the previous code and keep track of that around the loop? For the case of scaled (or indeed polynomial) offsets I'm not sure what I would pass to commonAlignment though as I can't simply pass in Offset.getKnownMinSize() in this case. I could do something like this perhaps:

commonAlignment(ST->getOriginalAlign(), Offset.isScalable() ? <something> : Offset.getFixedSize());

where in place of <something> I could introduce a new target query function that asks the target what the alignment should be? Or do you think it's better to leave the alignment unchanged like in the original code, i.e. always pass in ST->getOriginalAlign()?



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

https://reviews.llvm.org/D84937