Page MenuHomePhabricator

Add logical ops to Mips fast-isel
ClosedPublic

Authored by rkotler on Dec 10 2014, 5:28 PM.

Details

Summary

Code is mostly copied from AArch64 port and modified where needed for Mips.

This handles the "non" legal cases of logical ops. Legal cases are handled by tablegen patterns.

Diff Detail

Event Timeline

rkotler updated this revision to Diff 17133.Dec 10 2014, 5:28 PM
rkotler retitled this revision from to Add logical ops to Mips fast-isel.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler added a subscriber: rfuhler.
rkotler added a subscriber: Unknown Object (MLST).Dec 10 2014, 5:35 PM
dsanders accepted this revision.Dec 15 2014, 2:26 AM
dsanders edited edge metadata.

LGTM with whitespace nits and an improved test case. Currently many tests are missing and i32_test lacks any CHECK directives. There's a few more comments on this subject below.

lib/Target/Mips/MipsFastISel.cpp
232

Formatting nit: needs a blank line after this

236

Formatting nit: needs a blank line after this

239

Formatting nit: needs a blank line after this

243

Formatting nit: needs a blank line after this

245

Formatting nit: needs a blank line after this

479

Formatting nit: needs a blank line after this

734

Formatting nit: needs a blank line after this

748

Formatting nit: needs a blank line after this

750

Formatting nit: needs a blank line after this

test/CodeGen/Mips/Fast-ISel/logopm.ll
7–10

We don't need these lines

32–112

The tests are incomplete. Also, i32_test doesn't have any CHECK's.

The missing tests are:

  • i1 xor/or
  • i1 and/xor/or using immediates
  • i8 and/xor
  • i8 and/xor/or using immediates
  • i16 and/or
  • i16 and/xor/or using immediates
  • i32 and/xor/or (there's code for xor but no checks)
  • i32 and/xor/or using immediates (there's code for xor but no checks)

Also, please do one test per function (i.e. one function for the register form, and one for the immediate form). It makes it easier to tell what is covered.

Finally, test_i1 doesn't need to be significantly different from the others. Taking test_i8 and switching all the types to i1 (and switching to the correct global) will give you a simple test for 1-bit integers.

113–211

We don't need these lines. test/CodeGen tests are not executable.

This revision is now accepted and ready to land.Dec 15 2014, 2:26 AM
rkotler updated this revision to Diff 20368.Feb 19 2015, 8:13 PM
rkotler edited edge metadata.

This just brings the previous patch up to tip of tree. The test case will be replaced with a more complete test case and other comments from the previous review will be addressed.

rkotler updated this revision to Diff 20370.Feb 19 2015, 8:31 PM

FIx nits from previous review and rerun clang-format on the result.
What remains is to rewrite the test case.

Couple of inline comments.

Probably want to clean up the testcase a bit (though I know you've said you're working on it).

-eric

lib/Target/Mips/MipsFastISel.cpp
466

You never pass true for IsVectorAllowed, it's just dead so remove it.

test/CodeGen/Mips/Fast-ISel/logopm.ll
2

If you're going to line wrap then you should do it at 80-columns, this is just silly. :)

rkotler updated this revision to Diff 20710.Feb 25 2015, 3:46 PM

Finish up test case.
Merge to tip of tree and run clang format.
Will go through past comments to make sure all nits have been addressed and then will request final push approval.

rkotler updated this revision to Diff 20722.Feb 25 2015, 6:00 PM

Make Eric Christophers suggest change to remove the test for isVectorAllowed since this is not currently
an issue for Mips32 fast-isel... we don't support any vector types.

The code change looks right to me (assuming the answer to my main question below is 'no') but I have a couple questions and nits regarding the test case.

The main question is should we optimize 'X & 0', 'X | 0', and 'X ^ 0'?

test/CodeGen/Mips/Fast-ISel/logopm.ll
23–49

Nit: These three aren't testing anything. They should be deleted.

51–73

Just a comment, no change required: This test and the others like it would be a lot simpler if it used arguments and returns. The patches needed for this appear later in the series though so this is ok for now and we should clean it up after those patches land.

60

Nit: Please use CHECK-LABEL to partition the file

90

There's a really trivial optimization opportunity here (x & 0 == 0). It would simply require an 'if (Opc == Mips::AND && RHSReg == Mips::ZERO) return RHSReg' in emitLogicalOp().

I'm wondering if we should handle it or not given that the goal is compile time. I'm leaning towards yes since it's so cheap to catch. Do we still delete dead code when using Fast ISel? If so, I think it's probably best to handle it since it removes the need to encode instructions. What do you think?

113

Why not 'andi $[[RES1:[0-9]+]], $[[UB1]], 1'? Will andi support be added in a later patch?

154–160

Just a question: Any idea what optimized the 'X | 0'? I don't see anything in the patch that would do it and I'm surprised that it noticed this and didn't notice 'X & 0' or 'X ^ 0'.

229

Similar to the 'X & 0' case above, there's a trivial optimization opportunity here since 'X ^ 0 == X'. It would only require an 'if (Opc == Mips::XOR && RHSReg == Mips::ZERO) return LHSReg' in emitLogicalOp().

620–739

Nit: Commented out code is discouraged. It should be deleted.

rkotler added inline comments.Feb 27 2015, 10:34 AM
test/CodeGen/Mips/Fast-ISel/logopm.ll
32–112

I like to make the putback tests be executable and possibly removing the "main function" before putting them back so I'm sure that they work.

32–112

I32 is handled without this patch already by means of the tablegen patterns.

The need for this patch is to handle things that normally would be taken care of by the legalizer. For example: xor or any non 32 integer types.

90

This situation will not occur in real life. It's occurring here because you wanted me to test all these cases in the matrix and this code I've constructed by hand. If you start to test for all these things you will make the compiler slower and more complicated.
It's also not important at this time in the greater scheme of things which is not not let fast-isel fail and revert to non fast-isel.

113

People can use -O0 if they need better code here. The more things you add to complicated fast-isel the more unreadable it becomes and more prone to bugs.

154–160

I tried to make clang IR for the whole test matrix but sometimes there is something in llvm which is still optimizing something away.

229

Not relevant IMO to worry about this now or even in the future as I explained earlier.

620–739

This test case is executable and can be compared for results with non fast-isel for testing. It should really be added to test-suite. I do not want to lose this ability to later add this in the mips specific part of test-suite.

dsanders added inline comments.Mar 2 2015, 3:44 AM
test/CodeGen/Mips/Fast-ISel/logopm.ll
23–49

This hasn't been done.

32–112

I like to make the putback tests be executable and possibly removing the "main function" before putting them back so I'm sure that they work.

I agree that checking that we can execute real C/C++ code is a good thing to have in addition to the test/CodeGen tests. However, the test/CodeGen tests are unit tests for LLVM-IR to assembly. They are not intended to be executable or language-specific (although tests for specific bugs often will be) and redundant code in these tests just costs everyone additional test overhead without adding any value.

There are people working on adding executable test support though. I'm not sure what the current state of it is but the last I saw on the subject was '[LLVMdev] QEMU testing for LIT execution tests' in September. It's aimed at compiler-rt/libcxx/etc. testing but there's no reason trivial source->execution tests couldn't use the same mechanism. Maybe we could end up with a set of tests somewhere like test/CompileAndRun/C99/... or something along those lines.

I32 is handled without this patch already by means of the tablegen patterns.

We should still check the output assembly so that we know that the tablegen patterns do the right thing in Fast ISel. At the moment you have the test but don't check that it passes.

38–39

Something I missed on the previous read throughs: None the instructions with the immediate check that the immediate is there.

I understand that we don't emit 'andi $1, $2, 3' and similar yet but we should check that the immediate is materialized in a register that is used by the instruction that would have had it.

60

This hasn't been done

90

We wouldn't implement algebraic identity elimination at all if it didn't ever occur, but I do at least agree that likely to be infrequent.

We need some numbers to make the decision properly. Extra checks cost compile time but they may also reduce compile time further down the line (e.g. by removing instructions). Whether it's generally beneficial to have them or not, I don't know.

For now, let's focus on functionality as you say and investigate optimizations to reduce compile-time later.

113

Just to double check. Did you mean -O0 here?

Hmm. FastISel is certainly aimed at low compilation time but the execution time of the output is still a relevant factor. Avoiding the immediate form instructions seems like a sacrifice too far to me.

In any case, there's no need to add the immediate forms in this patch.

620–739

I have no problem with you adding it to a mips-specific part of the test-suite but it doesn't belong here.

dsanders added inline comments.Mar 2 2015, 7:19 AM
test/CodeGen/Mips/Fast-ISel/logopm.ll
32–112

I32 is handled without this patch already by means of the tablegen patterns.

We should still check the output assembly so that we know that the tablegen patterns do the right thing in Fast ISel. At the moment you have the test but don't check that it passes.

Please disregard this comment. It appears I've commented on the first revision and not the latest.

38–39

Something I missed on the previous read throughs: None the instructions with the immediate check that the immediate is there.

I understand that we don't emit 'andi $1, $2, 3' and similar yet but we should check that the immediate is materialized in a register that is used by the instruction that would have had it.

Please disregard this comment. I've commented on the first revision and not the latest.

rkotler updated this revision to Diff 21030.Mar 2 2015, 12:24 PM

Make requested changes to the test case.

Haven't looked at the contents of the testcases past the general stuff, but this looks much better.

One inline comment.

-eric

test/CodeGen/Mips/Fast-ISel/logopm.ll
582

Any reason for the CHECK-DAG here? Are you seeing ordering issues between mips32 and mip32r2 here?

rkotler added inline comments.Mar 3 2015, 11:35 AM
test/CodeGen/Mips/Fast-ISel/logopm.ll
582

The problem is that the load of US1_ADDR could be moved ahead of the load to US_ADDR. It's not happening today but there is no reason why the reordering could not occur and then it will create an unnecessary make check failure. I try and make heavy use of CHECK-DAG in order to ensure that only changes to the Mips target code generator will affect these tests and not changes to target independent code passes.

Thanks and sorry for the various delays from my side. LGTM.

rkotler updated this object.Mar 9 2015, 9:29 AM
rkotler edited the test plan for this revision. (Show Details)
rkotler closed this revision.Mar 9 2015, 9:30 AM