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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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);
 
----------------
Petar.Avramovic wrote:
> 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.
> 
At a minimum the correctness fix should be split out, but it seems like there are 3 separate changes here


================
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:
> 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.
If it doesn't matter for other targets and it's effectively the same, I don't care


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66533