[PATCH] D48374: [mips] Sign extend i32 return values on MIPS64

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:2140
+  /// combined in certain conditions.
+  virtual bool keepSextInRegNode(EVT VT) const {
+    return false;
----------------
smaksimovic wrote:
> efriedma wrote:
> > smaksimovic wrote:
> > > efriedma wrote:
> > > > Is this required for correctness?  If it is, then the MIPS target is lying to DAGCombine somehow, and this fix is just papering over the bug.  Otherwise, I can't see how keeping around a redundant SIGN_EXTEND_INREG would make the generated code more efficient.
> > > The sole purpose of that function, and keeping the SIGN_EXTEND_INREG node alive, is so that it could be later matched with this pattern:
> > > 
> > > ```
> > > def : MipsPat<(i64 (sext_inreg GPR64:$src, i32)),
> > >               (SLL64_64 GPR64:$src)>;
> > > ```
> > > As an example, SelectionDAG for the or_i32 function which resides in the or.ll test file:
> > > 
> > > ```
> > >   t0: ch = EntryToken
> > >             t2: i64,ch = CopyFromReg t0, Register:i64 %0
> > >           t4: i64 = AssertSext t2, ValueType:ch:i32
> > >         t5: i32 = truncate t4
> > >             t7: i64,ch = CopyFromReg t0, Register:i64 %1
> > >           t8: i64 = AssertSext t7, ValueType:ch:i32
> > >         t9: i32 = truncate t8
> > >       t10: i32 = or t5, t9
> > >     t11: i64 = sign_extend t10
> > >   t13: ch,glue = CopyToReg t0, Register:i64 $v0_64, t11
> > >   t14: ch = MipsISD::Ret t13, Register:i64 $v0_64, t13:1
> > > ```
> > > gets eventually transformed to this (including the sign_extend_inreg node incorporated by this patch):
> > > ```
> > >   t0: ch = EntryToken
> > >           t2: i64,ch = CopyFromReg t0, Register:i64 %0
> > >         t4: i64 = AssertSext t2, ValueType:ch:i32
> > >           t7: i64,ch = CopyFromReg t0, Register:i64 %1
> > >         t8: i64 = AssertSext t7, ValueType:ch:i32
> > >       t15: i64 = or t4, t8
> > >     t17: i64 = sign_extend_inreg t15, ValueType:ch:i32
> > >   t13: ch,glue = CopyToReg t0, Register:i64 $v0_64, t17
> > >   t14: ch = MipsISD::Ret t13, Register:i64 $v0_64, t13:1
> > > ```
> > > In place of sign_extend_inreg node would be the sign_extend node, which would be removed the same way the sign_extend_inreg would.
> > > Hence the keepSextInRegNode function to keep it until it can be pattern matched and replaced with the target dependent machine node.
> > > I completely agree that its existence is indeed questionable, looking at the code above by itself.
> > > However I have not found a better way to go about this change.
> > I don't understand: why do you want to generate a redundant "sll" instruction?
> Its purpose in this case is to sign extend the return value of a function, specifically i32 on MIPS64. At the moment that is not the case, even if the i32 return value is marked with the signext attribute.
> On MIPS, only the arithmetic instructions implicitly sign extend their results so we need to keep the sll instruction which does the extend for other operations prior to returning.
I don't mean "why do you want to generate an sll instruction in general"; I follow that part. I mean, "why do you want to generate a redundant sll instruction in your example"?  "i64 = or t4, t8" is already appropriately sign-extended.


https://reviews.llvm.org/D48374