[PATCH] D126085: [RISCV] Add a subtarget feature to enable unaligned loads

Wed May 25 11:44:46 PDT 2022

craig.topper added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11820

-  return false;
+  return Subtarget.enableUnalignedMem();
 }
----------------
reames wrote:
> reames wrote:
> > reames wrote:
> > > craig.topper wrote:
> > > > reames wrote:
> > > > > craig.topper wrote:
> > > > > > This seems like it should be a different feature for vectors.
> > > > > There might be such a target where scalar vs vector matters, but on the motivating case, this is not expected to matter.  We can split later if needed.
> > > > There are no vector tests and the description in the .td file says “scalar”. The patch should at least be self consistent.
> > > You're absolutely correct on that.  I'd originally had it split, then changed my mind.  Let me rebase and correct both of those!
> > It turns out writing vector tests was a useful exercise.  :)
> > 
> > From what I can tell, we aggressively canonicalize towards byte element loads and stores for vectors.  As such, no plain vector load or store is every treated as unaligned.
> > 
> > However, some indexed and strided load tests did fail if I asserted the fallthrough return was never taken.
> > 
> > For the moment, I decided to restrict the scope to only the scalar case.  Mostly because there look to be enough other opportunities around indexed loads that this didn't seem terribly useful.  
> > 
> > With that, I could go in two directions here.
> > 1) Rename this to scalar and add a separate flag later for vector cases as warranted.
> > 2) Document the intent that this covers all access, but leave the indexed/strided case currently unimplemented.
> > 
> > I have no strong preference and will do as reviewers request.  
> @craig.topper ping?
Let's rename it to scalar and add a separate flag for vector later as warranted.

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

https://reviews.llvm.org/D126085