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

Fri Feb 12 08:46:46 PST 2021

foad added inline comments.

================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1891
+  unsigned SrcBitWidth = SrcTy.getScalarSizeInBits();
+  MachineInstrBuilder LeftOperand, RightOperand;
+
----------------
Maybe assert that WideTy is at least twice as wide as SrcTy, otherwise the trick we use for calculating overflow below does not work.

================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulo.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=fiji -O0 -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=GFX8
+# RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx906 -O0 -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=GFX9
----------------
Can you use -check-prefixes=GCN,GFX8 and GCN,GFX9 so that update_mir_test_checks will common up the identical ones?

================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulo.mir:112
+    ; GFX8: [[TRUNC1:%[0-9]+]]:_(s16) = G_TRUNC [[COPY1]](s32)
+    ; GFX8: [[SEXT_INREG:%[0-9]+]]:_(s32) = G_SEXT_INREG [[TRUNC]], 16
+    ; GFX8: [[SEXT_INREG1:%[0-9]+]]:_(s32) = G_SEXT_INREG [[TRUNC1]], 16
----------------
This looks wrong as Matt noted above. Doesn't G_SEXT_INREG require identical source and result types? Would this fail MIR verification?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93963