[PATCH] D93963: [GlobalISel][AMDGPU] Lower G_UMULO/G_SMULO

[Pushpinder Singh via Phabricator via llvm-commits]

pdhaliwal added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1854-1855
+  } else {
+    // Unsigned overflow occurred if the high part is non-zero
+    auto ZExtResult = MIRBuilder.buildZExtInReg(WideTy, Mul, SrcBitWidth);
+    Overflow =
----------------
arsenm wrote:
> foad wrote:
> > pdhaliwal wrote:
> > > arsenm wrote:
> > > > I don't think the unsigned case is right. The DAG version inserts a shift here, not a mask
> > > I have changed it use shift instead of masking. Just curious, why was previous logic wrong? I thought zero'ing the upper bits of multiplication result and then comparing it with latter should provide the correct result.
> > I'm pretty sure the previous logic was fine too, it's just a different way of checking the upper part is zero.
> Oh yes, I just can't read. Arguably avoiding the shift is better since shifts can be more expensive
I have reverted to using masks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93963