implement SelectCmp (integer compare ) in mips fast-isel
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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 | |
| lib/Target/Mips/MipsFastISel.cpp | ||
|---|---|---|
| 531 |
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 |
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). | |
Comment Actions
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 | |
Comment Actions
- Add comment as TBD to revisit the possibility of eliminating redundant andi instructions after SelectCmp
- Refactor some code in the beginning of SelectCmp
Comment Actions
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]], 1The 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. | |
Comment Actions
Revision satisfying condition for LGTM.
Added comment to test case to look into possibility of removing redundant instruction during fast-isel code generation.
Style nit: This function needs some vertical spacing.