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

Thu Mar 28 19:50:05 PDT 2019

arsenm added a comment.

In D59971#1447031 <https://reviews.llvm.org/D59971#1447031>, @aemerson wrote:

> @arsenm Matt there are changes to the AMDGPU tests, but I'm not sure if they're ok or not. The tests run with -global-isel-abort=0 and the change of adding G_INSERT to the artifacts list means that it doesn't get legalized the same way (as an artifact, it doesn't get pushed onto the legalization list so it's deferred until later). Does it look benign?

The -global-isel-abort=0 is a workaround for the broken artifact handling. I didn't have any particular expectations for these other than not crashing

================
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;
----------------
I think this breaks the alignment. You should use MF.getMachineMemOperand(MMO, Size, Offset)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59971