This is an archive of the discontinued LLVM Phabricator instance.

[mips] Sign extend i32 return values on MIPS64
ClosedPublic

Authored by smaksimovic on Jun 20 2018, 9:00 AM.

Details

Summary

Override getTypeForExtReturn so that functions returning an i32 typed value have it sign extended on MIPS64.
Also provide patterns to get rid of unneeded sign extensions for arithmetic instructions which implicitly sign extend their results.

Diff Detail

Repository
rL LLVM

Event Timeline

smaksimovic created this revision.Jun 20 2018, 9:00 AM
This revision is now accepted and ready to land.Jun 21 2018, 10:47 PM
smaksimovic planned changes to this revision.Jul 4 2018, 8:30 AM

Forgot to mark this explicitly as still being WIP, since we are missing sign extensions at some places.

test/CodeGen/Mips/llvm-ir/or.ll
111 ↗(On Diff #152100)

For example here.
What is happening is that the already extended arguments which are operands to the 32 bit OR enable the operation to be transformed to a 64 bit OR through series of combines done at SimplifyBinOpWithSameOpcodeHands in DAGCombiner.cpp.
This in turn renders the sign_extend node introduced as a parent to the 64 bit OR unneeded so it gets removed, which leads to SLL instruction not being generated.

Likewise for the xor.ll test.

smaksimovic edited the summary of this revision. (Show Details)

Added target hooks to replace the sign_extend with the sign_extend_inreg node which will get pattern matched with the SLL64_64 thus providing the needed sign extension.
The second target hook is introduced to keep the sign_extend_inreg node from being combined away.

This revision is now accepted and ready to land.Jul 13 2018, 3:57 AM
efriedma added inline comments.
include/llvm/CodeGen/TargetLowering.h
2140 ↗(On Diff #155342)

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.

efriedma added inline comments.Jul 13 2018, 12:01 PM
test/CodeGen/Mips/llvm-ir/or.ll
111 ↗(On Diff #152100)

The sign extension here is redundant, I think: if x and y both have N sign bits, x | y also has N sign bits.

smaksimovic requested review of this revision.Jul 16 2018, 7:33 AM
smaksimovic added inline comments.
include/llvm/CodeGen/TargetLowering.h
2140 ↗(On Diff #155342)

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.

efriedma added inline comments.Jul 16 2018, 1:02 PM
include/llvm/CodeGen/TargetLowering.h
2140 ↗(On Diff #155342)

I don't understand: why do you want to generate a redundant "sll" instruction?

smaksimovic added inline comments.Jul 18 2018, 8:46 AM
include/llvm/CodeGen/TargetLowering.h
2140 ↗(On Diff #155342)

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.

efriedma added inline comments.Jul 18 2018, 11:00 AM
include/llvm/CodeGen/TargetLowering.h
2140 ↗(On Diff #155342)

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.

smaksimovic added inline comments.Jul 19 2018, 6:50 AM
include/llvm/CodeGen/TargetLowering.h
2140 ↗(On Diff #155342)

Ah, I see, there was a misunderstanding on my part.
I was under the impression, given the statement above regarding the arithmetic instructions and such, that I needed to sign extend the non-arithmetic ones unconditionally. Even in the example above the arguments to the operation were implicitly sign extended by the caller thus there is no need to additionally sign extend the result, as you noted. I'll revert this patch to the previous version.

Revert patch back to the previous version.

Is the patch in WIP state now?

Not anymore, I'd like to have it reviewed.
I rolled it back to the version similar to the one you reviewed initially.

This revision is now accepted and ready to land.Jul 26 2018, 2:59 AM
This revision was automatically updated to reflect the committed changes.