Page MenuHomePhabricator

implement integer compare in mips fast-isel
ClosedPublic

Authored by rkotler on Oct 1 2014, 3:22 PM.

Details

Summary

implement SelectCmp (integer compare ) in mips fast-isel

Diff Detail

Event Timeline

rkotler updated this revision to Diff 14297.Oct 1 2014, 3:22 PM
rkotler retitled this revision from to implement integer compare in mips fast-isel.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler added subscribers: mcrosier, rfuhler, Unknown Object (MLST).
rkotler updated this revision to Diff 14298.Oct 1 2014, 4:41 PM

Ran clang format on MipsFastISel.cpp

dsanders accepted this revision.Oct 3 2014, 3:11 AM
dsanders edited edge metadata.

LGTM with a few style and code duplication nits fixed along with an improvement to the testcase.

lib/Target/Mips/MipsFastISel.cpp
531

Style nit: This function needs some vertical spacing.

537–553

We've got some code duplication here. Both the LHS and the RHS are sign/zero extended in the same way, and we also duplicate the type checking that's inside EmitIntExt.

Can we factor it out?

611–626

These aren't needed. We already have a 'default: return false;'

test/CodeGen/Mips/Fast-ISel/icmpa.ll
22–23

Please check register numbers match between instructions using variables.

Likewise for the other tests below.

24

This instruction is redundant but we don't want to fix that in this patch. We might not want to fix it at all if it proves costly for compilation time. We should leave a comment on redundant instructions though.

Likewise for the others below

54

We ought to check that the operands appear in the right order. We can do this with variables by matching the loads first.

Likewise for the tests below

This revision is now accepted and ready to land.Oct 3 2014, 3:11 AM
dsanders added inline comments.Oct 6 2014, 6:34 AM
lib/Target/Mips/MipsFastISel.cpp
531

What kind of vertical spacing? I'm using clang-format on this file.

I mean it needs some blank lines to break up the logical sections of the code. For example, a blank line between the LHS/RHS handling, and just before the switch would help readability.

537–553

What do you mean about duplicating tests inside of EmitIntExt?
Not sure what you are referring to.

At the moment, the caller is testing that LMVT is i8 or i16 and passes LMVT to EmitIntExt as the first argument. EmitIntExt also checks that first argument type is i8 or i16 (in its switch statement). It seems unnecessary to do it twice and have to maintain the list in both places. One possible solution is to call EmitIntExt unconditionally and have it simply return the input register when an extension isn't necessary (e.g. %1 = sext i32 %0 to i32).

rkotler updated this revision to Diff 14483.Oct 6 2014, 6:41 PM
rkotler edited edge metadata.

Make requested changes from previous review.

rkotler updated this revision to Diff 14519.Oct 7 2014, 9:59 AM

Fix some cut and paste mistakes in the test case.

Thanks. Your update fixes the most important comments but you've missed a couple. I've replied to each comment indicating whether I think it's been done or not.

lib/Target/Mips/MipsFastISel.cpp
531

This comment hasn't been handled by your update

537–553

Reed and I have chatted about the duplicate type checks and we've agreed to follow up on this in a later patch. This is because it requires refactoring EmitIntExt() so it supports i32->i32 without creating a temporary register.

The remaining part of this comment hasn't been handled by the update though. The LHS/RHS code duplication I'm referring to is:

if (LeftReg == 0)
  return false;
MVT LMVT = TLI.getValueType(Left->getType(), true).getSimpleVT();
if ((LMVT == MVT::i8) || (LMVT == MVT::i16)) {
  unsigned TempReg = createResultReg(&Mips::GPR32RegClass);
  if (!EmitIntExt(LMVT, LeftReg, MVT::i32, TempReg, IsUnsigned))
    return false;
  LeftReg = TempReg;
}

then following it is the same code with s/LeftReg/RightReg/ and s/LMVT/RMVT/. If it were only couple lines then I'd leave it but there's enough there that I'd prefer not to duplicate it.

611–626

Done

test/CodeGen/Mips/Fast-ISel/icmpa.ll
22–23

Done

24

This comment hasn't been handled.

54

Done

rkotler updated this revision to Diff 14584.Oct 8 2014, 11:46 AM
  1. Add comment as TBD to revisit the possibility of eliminating redundant andi instructions after SelectCmp
  2. Refactor some code in the beginning of SelectCmp

LGTM with the redundant instruction comments added to the test.

lib/Target/Mips/MipsFastISel.cpp
531

Done

537–553

Done. When you do follow up on the duplicate type checks in a later patch, I'm expecting most of getRegEnsuringSimpleIntegerWidening() to end up folded into EmitIntExt(). Please follow up on this fairly soon.

test/CodeGen/Mips/Fast-ISel/icmpa.ll
24

I think you've misunderstood me. The comment you've added to the implementation is good but I actually intended for a comment to be left on the redundant instructions of these tests. As in:

; FIXME: This instruction is redundant. The sltiu can only produce 0 and 1.
; CHECK: andi ${{[0-9]+}}, $[[REG2]], 1

The aim is to clearly mark parts of the test that are suboptimal or undesirable but we've temporarily compromised on. This helps us to remember issues we need to come back to and also helps anyone who accidentally affects this test in some way.

rkotler updated this revision to Diff 14737.Oct 10 2014, 10:06 AM

Revision satisfying condition for LGTM.

Added comment to test case to look into possibility of removing redundant instruction during fast-isel code generation.

rkotler closed this revision.Oct 10 2014, 10:50 AM