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

[Philip Reames via Phabricator via llvm-commits]

reames marked an inline comment as not done.
reames added inline comments.


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


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

https://reviews.llvm.org/D126085