[PATCH] D150717: [RISCV] Use scalar stores for splats of zero to memory up to XLen

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:12189
+      auto NewVT = MVT::getIntegerVT(MemVT.getSizeInBits());
+      if (allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
+                                         NewVT, *Store->getMemOperand())) {
----------------
asb wrote:
> luke wrote:
> > Do we want to check if the memory access is fast? Currently we always report unaligned scalar accesses as slow, but that sounds like it's at odds with this combine
> > 
> > ```
> > bool RISCVTargetLowering::allowsMisalignedMemoryAccesses(
> >     EVT VT, unsigned AddrSpace, Align Alignment, MachineMemOperand::Flags Flags,
> >     unsigned *Fast) const {
> >   if (!VT.isVector()) {
> >     if (Fast)
> >       *Fast = 0;
> >     return Subtarget.enableUnalignedScalarMem();
> >   }
> > ```
> Luke and I chatted about this earlier today, and the specific case would be where a vector access that is aligned according to the natural alignment of its elements is converted to a wider scalar access that is misaligned. Though now reminding myself of the how +unaligned-scalar-mem is used (https://reviews.llvm.org/D126085), this is probably not a concern, as it's only enabled if unaligned scalar mem is performant enough to be worth using.
Honestly, this is whole area is a bit of a mess.  What exactly the fast flag means (it's a unsigned so it has many possible states) is very poorly defined.  It's also extremely target specific, but interacts with generic codegen, so the best kind of fun.

At the moment, we're interpreting it as basically a hint that misaligned accesses should be aggressively formed - as opposed to simply used when natural during lowering.  

In this case, I think moving forward without Fast is the right practical answer, but this is a bit of a code smell.

Thanks for pointing this out, I'd not considered it.  I'm going to be looking at a couple other aspects of mem-op lowering, and I'll see if I can clean this up.


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

https://reviews.llvm.org/D150717