[PATCH] D55814: GlobalISel: Support narrowing zextload/sextload

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.
Herald added a subscriber: volkan.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:526
+    auto &MMO = **MI.memoperands_begin();
+    if (MMO.getSize() * 8 == NarrowSize) {
+      MIRBuilder.buildLoad(TmpReg, PtrReg, MMO);
----------------
arsenm wrote:
> aemerson wrote:
> > Is this really needed?
> I’m not sure what you mean? Otherwise you’ll end up with an extload with the same source and dest size (which I think the verifier should reject)
I agree the verifier should reject it. 's32 = G_ZEXTLOAD ... :: (load 4 from ...)' isn't exactly wrong but it serves no purpose compared to 's32 = G_LOAD ... :: (load 4 from ...)'


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:540
+    else
+      MIRBuilder.buildSExt(DstReg, TmpReg);
+
----------------
However, this should be invalid for the same reason, 's32 = G_SEXT s32' doesn't make sense. It would be better for the case where it lowers to G_LOAD to just replace the opcode in place. If we have observers plumbed into the legalizer yet we should also call the changingInstr()+changedInstr() events.


================
Comment at: lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:210
+  ExtLoads.clampScalar(0, S32, S32)
+          .widenScalarToNextPow2(0)
+          .unsupportedIfMemSizeNotPow2()
----------------
This is redundant given that clampScalar() limits the type to exactly s32. It's nice for consistency but it costs a small amount of compile-time to perform the test.


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

https://reviews.llvm.org/D55814