Page MenuHomePhabricator

[mips][FastISel] Implement shift ops for Mips Fast-Isel.
ClosedPublic

Authored by vkalintiris on Dec 18 2014, 11:30 AM.

Details

Summary

Add shift operators implementation to Fast-Isel for Mips. These are shift ops
for non legal forms, i.e. i8 and i16.

Based on a patch by Reed Kotler.

Diff Detail

Repository
rL LLVM

Event Timeline

rkotler updated this revision to Diff 17458.Dec 18 2014, 11:30 AM
rkotler retitled this revision from to Implement shift ops for 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: Unknown Object (MLST), rfuhler.
dsanders edited edge metadata.Jan 11 2015, 9:50 AM

Testcase? Also, one common nit is that many of the comments lack capitalization.

From the patch summary:

Am reviewing this myself now but passes all of test-suite.

The patch summary is supposed to be the commit message you intend to use. This kind of thing ought to be a follow up comment.

lib/Target/Mips/MipsFastISel.cpp
1304–1324 ↗(On Diff #17458)

Can you show me a test case that requires this? At the moment, the old code sounds correct to me and I think the consumers of this value should be sign/zero extending if they care about particular values of the undefined portion of the register.

1430 ↗(On Diff #17458)

The first section of code needs some vertical whitespace to break up the logical parts. It's a page of solid code at the moment.

1443–1449 ↗(On Diff #17458)

Just a comment:
I expect a lot of clutter could be eliminated by moving the createResultReg() and associated code inside the emit*() functions and returning the register or 0 for failure.

Then this code here would be something like:

MVT Op0MVT = ...;
Op0Reg = emitIntSExt(Op0MVT, ...);
if (!Op0Reg)
  return false;

It also allows the emit functions to handle nops by returning an operand register and constant 0 by returning $zero which will be useful in some cases.

1451–1452 ↗(On Diff #17458)

Shouldn't this comment come just before the if-statement on line 1442?

Also, I don't think this is limited to AShr and we don't need to do it if the operand came from a SExtInst. I think LShr ought to be zero extending unless the operand comes from a ZExtInst.

1463–1465 ↗(On Diff #17458)

Is this comment still true? You already have sign extension code for operand 0 above.

rkotler updated this revision to Diff 20797.Feb 26 2015, 3:20 PM
rkotler updated this object.
rkotler edited edge metadata.

Freshen up patch to tip of tree.

Hi Reed,

Inline comments. Also, everything Daniel said.

-eric

lib/Target/Mips/MipsFastISel.cpp
1297 ↗(On Diff #20797)

I'd add this assert:

assert((DVT.SimpleTy != MVT::i32 && DVT.SimpleTy != MVT::i64) && "i64 and i32 should have been handled by target-independent fast-isel!");

1448 ↗(On Diff #20797)

Is this an actual error or what?

1473 ↗(On Diff #20797)

Is this an actual error or what?

I am working on the make check test. Attached is the C code that was the original test case.

lib/Target/Mips/MipsFastISel.cpp
1304–1324 ↗(On Diff #17458)

This is definitely there to fix some failure in test-suite that will show up without this. It's clearly not incorrect to add this code. The whole way that fast-isel skirts around not having a legalizer is dubious at times. I don't know how easy it will be to create a separate test case for this. So it's a problem with an earlier patch but only shows up if you add this patch to it.

So we clearly cannot check in the rest of the patch without this because test-suite will have failures. So I consider it to be part of implementing this patch.

1304–1324 ↗(On Diff #17458)

My recollection is that other fast-isel ports do not implement the shift ops for non legalized types which is why they are not seeing this issue.

1451–1452 ↗(On Diff #17458)

Yes, the comment needs to be moved above.

I don't think we need sign extension unless we have AShr. What is in the register is already an 8, 16 bit value. It's as if there was an 8, 16 bit register. These are treated as if they are legal types but they are not really legal. We want to use a 32 bit form of shift which will have the right semantics unless it's an AShr.

In the case of an Ashr, it will not behave properly unless we have sign extended the value into a 32 bit register.

This patch BTW passes all of test suite and there are tests where getting this semantics right counts.

1463–1465 ↗(On Diff #17458)

Correct. The comment does not belong anymore.

The C code for the test case was modified. I will finish it up tomorrow.

This patch is so old I forgot that I had to modify it to create the proper clang IR for this test.

rkotler updated this revision to Diff 20897.Feb 27 2015, 2:09 PM
rkotler edited the test plan for this revision. (Show Details)

Add test case/

rkotler added inline comments.Feb 27 2015, 3:22 PM
lib/Target/Mips/MipsFastISel.cpp
1297 ↗(On Diff #20797)

If we want to add that then why not just a simple LLVM_Unreachable ??

1448 ↗(On Diff #20797)

I think it should be impossible to get that because we are only calling this function upon seeing one of those instructions. I think I added that case to avoid compiler warnings when compiling this.

In fastSelectionInstruction

case Instruction::Shl:
case Instruction::LShr:
case Instruction::AShr:
  return selectShift(I);
1473 ↗(On Diff #20797)

I think it should be impossible to get that because we are only calling this function upon seeing one of those instructions. I think I added that case to avoid compiler warnings when compiling this.

Replies inline.

-eric

lib/Target/Mips/MipsFastISel.cpp
1297 ↗(On Diff #20797)

Because SimpleTy are more than the two you have there and you'd like to make sure that all of the possible types that come in are handled rather than say that you've handled all of them?

1448 ↗(On Diff #20797)

Makes sense. Go ahead and make your comment more descriptive of what instructions you expect to see.

dsanders added inline comments.Mar 16 2015, 7:57 AM
lib/Target/Mips/MipsFastISel.cpp
1304–1324 ↗(On Diff #17458)

A separate test case is ideal, but pointing me at the test-suite failure and the bad assembly within it will suffice.

I'm not saying that I think the zero extension is unnecessary, I just suspect that the wrong LLVM-IR expansion is doing it.
My suspicion is that the consumer of the value is reading the whole register instead of either forcing a zero extension itself or reading just the bits it expects to operate on. It could well be selectShift() since it currently doesn't zero-extend the operand for a LShr.

For example:

%0 = trunc i32 %a to i16
%1 = trunc i32 %b to i16
%2 = add i16 %0, %1

clearly doesn't need either of the zero extensions this code will currently create but:

%0 = trunc i32 %a to i16
%1 = trunc i32 %b to i16
%2 = lshr i16 %0, i32 3

needs lshr to zero extend the input register before use.

1451–1452 ↗(On Diff #17458)

Also, I don't think this is limited to AShr and we don't need to do it if the operand came from a SExtInst. I think LShr ought to be zero extending
unless the operand comes from a ZExtInst.

I don't think we need sign extension unless we have AShr. What is in the register is already an 8, 16 bit value. It's as if there was an 8, 16 bit
register. These are treated as if they are legal types but they are not really legal. We want to use a 32 bit form of shift which will have the right
semantics unless it's an AShr.

In the case of an Ashr, it will not behave properly unless we have sign extended the value into a 32 bit register.

I agree with the arguments and explanation here but I think you may have missed my key question. The question I'm trying to ask is: Why doesn't LShr zero extend the input register just like AShr sign extends the input register? Similarly to AShr it will be implementing this with a 32-bit shift right and surely therefore requires zero extension to i32 to happen first. I suspect the reason this currently passes the test suite is because the code in selectTrunc() that I've raised comments about is concealing the bug in selectShift() for LShr.

The secondary part of the comment is that we also don't need to sign-extend if the operand in question is already a suitable sign-extend operation.

1297 ↗(On Diff #20797)

I assume Reed meant something like:

default:
  return false;
case MVT::i32:
case MVT::i64:
  llvm_unreachable("Text");
  return false;

I was recently informed that llvm_unreachable() is pretty much the same thing as assert(0 && "Text") and that they are removed in release builds (and even informs the optimizer that the path is unreachable). On that basis, an assert() is slightly preferable to a llvm_unreachable().

vkalintiris commandeered this revision.Apr 16 2015, 2:51 AM
vkalintiris added a reviewer: rkotler.

Sign/Zero extend operand for right arithmetic/logical shifts.

Vasileios is going to be taking over our Fast ISel work since Reed is needed elsewhere. Vasileios's (and my) first priority is to clear out the patch backlog, then he'll move on to cleaning things up and removing some unnecessary restrictions (e.g. PIC is currently required even for operations that don't involve addresses).

@vkalintiris: Thanks for sorting out the inconsistency between sign/zero-extension handling. That was the main issue I had with the patch.

Could you change the subject and description to be more commit-message like? Also, could you add something like 'Based on a patch by Reed Kotler' or 'Patch by Reed Kotler with some changes by Vasileios Kalintiris'. Reed should still get credit for his work.

lib/Target/Mips/MipsFastISel.cpp
1390 ↗(On Diff #23836)

Nit: Could you add some appropriate vertical whitespace here? It's quite a long block of code at the moment.

1402–1403 ↗(On Diff #23836)

Nit: Capitalization and punctuation. Also the second comment line is blank

test/CodeGen/Mips/Fast-ISel/shftopm.ll
20 ↗(On Diff #23836)

Nit: Could you switch this over to CHECK-LABEL? Also, it's more usual to check for 'sll:'.

29–30 ↗(On Diff #23836)

Nit: Indentation. Likewise for a few other cases below

31 ↗(On Diff #23836)

Nit: Not necessary if we use CHECK-LABEL for the start label.

136–208 ↗(On Diff #23836)

Nit: Please remove this. It's not adding anything to the test.

209–217 ↗(On Diff #23836)

Nit: We should be able to remove this too.

vkalintiris retitled this revision from Implement shift ops for Mips fast-isel to [mips][FastISel] Implement shift ops for Mips Fast-Isel..
vkalintiris updated this object.

Addressed nits.

dsanders accepted this revision.Apr 17 2015, 6:39 AM
dsanders edited edge metadata.

LGTM, Thanks.

This revision is now accepted and ready to land.Apr 17 2015, 6:39 AM
This revision was automatically updated to reflect the committed changes.