This is an archive of the discontinued LLVM Phabricator instance.

[mips] Promote the result of SETCC nodes to GPR width.
ClosedPublic

Authored by vkalintiris on Jul 6 2015, 12:27 PM.

Details

Summary

This patch modifies the existing comparison, branch, conditional-move
and select patterns, and adds new ones where needed. Also, the updated
SLT{u,i,iu} set of instructions generate a GPR width result.

The majority of the code changes in the Mips back-end fix the wrong
assumption that the result of SETCC nodes always produce an i32 value.
The changes in the common code path account for the fact that in 64-bit
MIPS targets, i1 is promoted to i32 instead of i64.

Diff Detail

Event Timeline

vkalintiris retitled this revision from to [mips] Promote the result of SETCC nodes to GPR width..
vkalintiris updated this object.
vkalintiris added a reviewer: dsanders.
vkalintiris added a subscriber: llvm-commits.
dsanders requested changes to this revision.Oct 7 2015, 8:32 AM
dsanders edited edge metadata.

Sorry for taking so long to get to this one.

It's generally looking good. Most of my comments are nits, minor issues, and questions.

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
1078

This should probably be picking registers from FGRCC64 rather than FGRCC32.

lib/Target/Mips/Mips32r6InstrInfo.td
758–831

Just to make the patch a bit more readable: Could you separate the change to the multiclass style into another patch?

Also, indentation and unnecessary reordering. (the contents of Cmp_Pats used to be at the start of this hunk)

lib/Target/Mips/Mips64r6InstrInfo.td
78–79

This is because operands are still 32-bit instead of GPR-width, right?

111–112

ISA_MIPS32R6 and GPR_64 together is only possible for ISA_MIPS64R6.

I see that there are four pre-existing cases of ISA_MIPS32R6 in this file and they look like bugs to me.

113

Why codegen only?

115

FIELD_CMP_FORMAT_S should be FIELD_CMP_FORMAT_D

lib/Target/Mips/MipsCondMov.td
157–176

iPTR is i32 on N32 but mov[zn] is still a 64-bit move.

179–180

This can fit on one line. Similarly below

lib/Target/Mips/MipsISelLowering.cpp
643–644

I think this should be more general. E.g.:

if (SetCC.getValueType().getSizeInBits() > False.getValueType().getSizeInBits())
  SetCC = DAG.getNode(ISD::TRUNCATE, DL, False.getValueType, SetCC);
659–660

Likewise

1709–1710

I don't see the reason for this assert at the moment. Is it a requirement of createCMovFP()? If so, shouldn't it be asserted inside that function?

lib/Target/Mips/MipsRegisterInfo.td
394

'This class allows' -> 'These classes allow'

test/CodeGen/Mips/atomic.ll
351

Do you know why we still get this redundant sign extend? I was expecting it to disappear once the result type was i64.

test/CodeGen/Mips/cmov.ll
469–472

Is this still true?

test/CodeGen/Mips/countleading.ll
7

Well spotted

test/CodeGen/Mips/fcmp.ll
36

I think you're missing the signext attribute on the result of the function.

Likewise below

45

This one is doubly-redundant. cmp.eq.s produces GPR-width 0 and -1 and we're also about to zero-extend 1-bit to GPR-width. I'm surprised the latter reason didn't remove it.

test/CodeGen/Mips/llvm-ir/select.ll
158

Why remove this signext? Likewise below.

This revision now requires changes to proceed.Oct 7 2015, 8:32 AM
vkalintiris edited edge metadata.
vkalintiris marked an inline comment as done.

WIP - Clean-up patch to make it diff-friendly.

vkalintiris marked 10 inline comments as done.
vkalintiris edited edge metadata.

Addressed reviewer comments.

lib/Target/Mips/MipsISelLowering.cpp
1709–1710

I forgot to remove this before creating the review request. I wrote the assert when I was developing this patch because it fired in one case.

test/CodeGen/Mips/atomic.ll
351

In this case, it's a truncation from i64 to i32. There's a problem with RetCC_MipsN at the moment and we use V0 instead of V0_64. We'll have to fix this in a follow-up patch. The same is true for the rest of the truncations (SLLs) in rest of your questions.

test/CodeGen/Mips/cmov.ll
469–472

No, I added two new test cases.

vkalintiris updated this revision to Diff 41639.Dec 2 2015, 9:15 AM

Rebased & fixed sel.{s,d} commands for microMIPS{32,64}r6.

I think we're nearly there. Most of these are either minor issues or nits.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13479

Looking at this again, I notice that getZExtOrTrunc() can only produce ZERO_EXTEND or TRUNCATE. We lose the getZeroExtendInReg() path. Could you bring back the special case?

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
956

Could you tell me which case made this necessary?

lib/Target/Mips/MicroMips64r6InstrInfo.td
125–127 ↗(On Diff #41639)

Indentation

lib/Target/Mips/Mips64r6InstrInfo.td
78–79

This is because operands are still 32-bit instead of GPR-width, right?

Could you reply to this comment?

113

Naming nit: I think this should be 'Mips64r6' since these instruction definitions are not valid for Mips32r6.

118–119

80 cols?

lib/Target/Mips/MipsCondMov.td
265–274

This isn't quite right. iPTR is i32 for N32 but comparisons should produce i64.

290–292

This isn't quite right. iPTR is i32 for N32 but comparisons should produce i64.

lib/Target/Mips/MipsISelLowering.cpp
271–273

It ought to be based on the ABI rather than the architecture. Can you use MipsABIInfo::AreGprs64bit()?

474

It ought to be based on the ABI rather than the architecture. Can you use MipsABIInfo::AreGprs64bit()?

3936

It ought to be based on the ABI rather than the architecture. Can you use MipsABIInfo::AreGprs64bit()?

lib/Target/Mips/MipsSEISelLowering.h
71–72

Did you mean to include this? There's no implementation.

test/CodeGen/Mips/atomic.ll
351

Ah ok. This is the problem you showed me last week where the N32 calling convention is being partially handled by the O32 implementation.

Do you think it's best to fix that before or after this patch?

test/CodeGen/Mips/llvm-ir/select.ll
158

Why remove this signext? Likewise below.

Could you reply to this comment?

vkalintiris marked 14 inline comments as done.
  • Fix conditional-move patterns to use i64 comparisons on N32/N64 instead of iPTR.
  • Restore PromoteIntOp_ANY_EXTEND() to its original state and solve the problem during DAG's building.
  • Address a few other nits.
vkalintiris added inline comments.Dec 17 2015, 4:01 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13479

With this input:

(select (i64 (setcc ...)), (i32 N1), (i32 N2))

We'll get SCC:i64 and Temp:i64. The ISD::SHL node below will try to create a shift operation with:

(i32 (shl i64, i32))

which is not valid. Given that the boolean contents are either all 0 or all 1, we can use getZextOrTrunc(), right?

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
956

The initial problem came from test/Codegen/atomic.ll:

t17: i8,i1,ch = AtomicCmpSwapWithSuccess<Volatile LDST1[%ptr]> t0, t2, t14, t16
  t18: i32 = any_extend t17:1
t20: ch,glue = CopyToReg t17:2, Register:i32 %V0, t18
t21: ch = MipsISD::Ret t20, Register:i32 %V0, t20:1

The boolean result of AtomicCmpSwapWithSuccess will be promoted to i64 by the call to GetPromotedInteger() in the previous line.

I decided to fix this by consulting getSetCCResultType() in the initial SelectionDAG builder when we construct an AtomicCmpSwapWithSuccess node.

lib/Target/Mips/Mips64r6InstrInfo.td
78–79

Sorry, I missed this. You're correct, that's why we have to use a custom inserter.

test/CodeGen/Mips/atomic.ll
351

I've already started writing the necessary patches in order to fix this. I believe it would be better to submit this patch first, and deal with the N32/N64 problem later.

test/CodeGen/Mips/llvm-ir/select.ll
158

Sorry, I missed this. I wanted to minimize the number of changes that I had to do in the test cases. The patch that fixes the N32/N64 ABI problem rewrites a lot of these tests and adds the signext back.

Use getTypeToTransformTo() when the type from getSetCCResultType() is *not*
legal.

Thanks for your patience on this, I think we're nearly there. Just a couple missed comments and a bit more on the removal of the ZERO_EXTEND_INREG case.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13479

That's not what I'm getting at. Previously, we had two cases ZERO_EXTEND and ZERO_EXTEND_INREG. This patch adds the possibility of TRUNCATE which makes sense to me. However, it also changes the ZERO_EXTEND_INREG cases to ZERO_EXTEND and it's this removal that I don't understand.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
956

Ok, that makes sense to me.

lib/Target/Mips/MicroMipsInstrInfo.td
629–633

Indentation (GPR32Opnd should line up with the ")

test/CodeGen/Mips/atomic.ll
351

Makes sense to me.

test/CodeGen/Mips/fcmp.ll
36

Could you comment on this?

45

Could you comment on this?

test/CodeGen/Mips/llvm-ir/select.ll
158

Ok then. I think answers one of my questions for that patch too.

vkalintiris marked 7 inline comments as done.

Rebase & addressed comments.

vkalintiris added inline comments.Feb 29 2016, 7:12 AM
test/CodeGen/Mips/fcmp.ll
36

My earlier comment covers both cases:

In this case, it's a truncation from i64 to i32. There's a problem with RetCC_MipsN at the moment and we use V0 instead of V0_64. We'll have to fix this in a follow-up patch. The same is true for the rest of the truncations (SLLs) in rest of your questions.

The aforementioned RetCC patch is in D16220.

dsanders accepted this revision.Feb 29 2016, 8:46 AM
dsanders edited edge metadata.

LGTM. Thanks for working on this.

This revision is now accepted and ready to land.Feb 29 2016, 8:46 AM
This revision was automatically updated to reflect the committed changes.