[PATCH] D59971: [GlobalISel] Add legalization support for non-power-2 loads and stores

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked an inline comment as done.
aemerson added a comment.

In D59971#1467947 <https://reviews.llvm.org/D59971#1467947>, @arsenm wrote:

> In D59971#1467546 <https://reviews.llvm.org/D59971#1467546>, @aemerson wrote:
>
> > Reviving this as the overall approach was fine, it seems the alignment of non pow2 types is assumed to be the alignment of the next largest pow-2 type, so we don't need to worry about alignment during the breakdown.
> >
> > I did however change the legalization method to not use extracts/inserts, but instead use extending loads and truncating stores, so that the artifacts get combined away and it Just Works.
>
>
> I don't like how we do everything in bits, and then the mem operand forces bytes. Would it cost anything to switch MemOperands to also be in bits?


Yes I think we could introduce a getSizeInBits() accessor to make it clearer, but I think it should be a separate patch from this.



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1379-1389
+  MachineMemOperand *NewLargeMMO = MF.getMachineMemOperand(
+      MMO->getPointerInfo(), MMO->getFlags(), Size1 / 8, MMO->getAlignment(),
+      MMO->getAAInfo(), MMO->getRanges(), MMO->getSyncScopeID(),
+      MMO->getOrdering(), MMO->getFailureOrdering());
+
+  MachinePointerInfo SmallPtrInfo = MMO->getPointerInfo();
+  SmallPtrInfo.Offset = Size1 / 8;
----------------
arsenm wrote:
> arsenm wrote:
> > arsenm wrote:
> > > I think this breaks the alignment. You should use MF.getMachineMemOperand(MMO, Size, Offset)
> > I got the order wrong, it's:
> >  
> > ```
> > MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
> >                                           int64_t Offset, uint64_t Size);
> > ```
> This is still using the new function?
It should do, I forgot to make that change in the latest patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59971