[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:11810
   if (!VT.isVector())
-    return false;
+    return Subtarget.enableUnalignedMem();
 
----------------
jrtc27 wrote:
> If returning true and Fast isn't null we should be setting it; some users pre-initialise it, but not all (I found one in GlobalISel and stopped looking)
Good catch.  I will unconditionally initialize to false to make sure we don't miss any paths.  Thanks!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126085