[PATCH] D66533: [MIPS GlobalISel] ClampScalar G_SHL, G_ASHR and G_LSHR

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic marked 4 inline comments as done.
Petar.Avramovic added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2993
     // Short: ShAmt < NewBitSize
-    auto LoS = MIRBuilder.buildShl(HalfTy, InH, Amt);
+    auto LoS = MIRBuilder.buildShl(HalfTy, InL, Amt);
 
----------------
arsenm wrote:
> This looks like it was copied incorrectly from the DAG version. Can you commit this part separately?
The whole change in LegalizerHelper.cpp and tests including merge of G_ASHR and G_LSHR into same case? OrLHS for G_ASHR also had typo, it used buildAShr instead of buildShl.



================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3001
     auto LoL = MIRBuilder.buildConstant(HalfTy, 0);         // Lo part is zero.
-    auto HiL = MIRBuilder.buildShl(HalfTy, InL, AmtExcess); // Hi from Lo part.
+    auto HiL = MIRBuilder.buildShl(HalfTy, InL, Amt);       // Hi from Lo part.
 
----------------
Petar.Avramovic wrote:
> arsenm wrote:
> > This now differs from the DAG version?
> Oh, now I see it, didn't look into DAG version, I was guided by the test.
> Will have to double check this.
MIPS has custom legalize for shifts in SDAG so that is target optimization since shift for Amt and AmtExcess is effectively the same thing since only low 5 bits are considered for shift amount. Will return AmtExcess here.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3014-3016
+    auto HiS = MIRBuilder.buildInstr(MI.getOpcode(), {HalfTy}, {InH, Amt});
+    auto OrLHS = MIRBuilder.buildShl(HalfTy, InH, AmtLack);
+    auto OrRHS = MIRBuilder.buildLShr(HalfTy, InL, Amt);
----------------
arsenm wrote:
> As far as I can tell this is ultimately still the same as the DAG version?
yes


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66533