[PATCH] D33442: [ARM] Fix lowering of misaligned memcpy/memset

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4782
   if (VT == MVT::Other) {
+    EVT PointerVT = TLI.getPointerTy(DAG.getDataLayout(), DstAS);
     if (DstAlign >= DAG.getDataLayout().getPointerPrefAlignment(DstAS) ||
----------------
efriedma wrote:
> The use of getPointerTy() here seems dubious. On many architectures, you can use getPointerTy as a rough proxy for the largest legal integer type, but that's not universal, and the usage of DstAS doesn't make any sense.
> 
> Maybe just loop over {i64, i32, i16}.
I'd thought about doing that but instead went with the simpler change. I'll give the loop approach a try.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4796
     MVT LVT = MVT::i64;
     while (!TLI.isTypeLegal(LVT))
       LVT = (MVT::SimpleValueType)(LVT.SimpleTy - 1);
----------------
efriedma wrote:
> The use of isTypeLegal() here looks suspicious; i16 is not legal on ARM, but we definitely want to use i16 stores if we can.  Do you have any testcases where the known alignment is two?
This loop is actually finding the largest legal type, i.e. i32 on ARM, so that if VT is larger than that it's used instead. Adding a test sounds like a good idea though.


Repository:
  rL LLVM

https://reviews.llvm.org/D33442