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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11820
 
-  return false;
+  return Subtarget.enableUnalignedMem();
 }
----------------
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.  


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

https://reviews.llvm.org/D126085