Page MenuHomePhabricator

Add sign/zero extend to mips fast-isel
AbandonedPublic

Authored by rkotler on Aug 7 2014, 5:19 PM.

Details

Reviewers
rfuhler
dsanders
Summary

Add sign/zero extend to mips fast-isel
(Rewrite of D3984)

Diff Detail

Event Timeline

rkotler updated this revision to Diff 12298.Aug 7 2014, 5:19 PM
rkotler retitled this revision from to D3984.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler retitled this revision from D3984 to Add sign/zero extend to mips fast-isel.Aug 7 2014, 5:21 PM
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
dsanders edited edge metadata.Aug 8 2014, 9:40 AM

Seeing as you're starting a new review. Could you mark D3984 abandoned?

lib/Target/Mips/MipsFastISel.cpp
252–264

I think we could better write this as something like:

unsigned SizeInBits = SrcVT.getSizeInBits();
if (SizeInBits > GPRWidthInBits)
  return false;
if (SizeInBits == 8 && isMips32r2()) {
  EmitInst(Mips::SEB, DestReg).addReg(SrcReg);
  return true;
}
if (SizeInBits == 16 && isMips32r2()) {
  EmitInst(Mips::SEH, DestReg).addReg(SrcReg);
  return true;
}
EmitInst(Mips::SLL, DestReg).addReg(SrcReg).addImm(GPRWidth - SizeInBits); 
EmitInst(Mips::SRA, DestReg).addReg(DestReg).addImm(GPRWidth - SizeInBits);
return true;

rather than listing each case. This looks a bit longer but it handles MIPS32r1/MIPS64r2 (and earlier), as well as unusual types such as i11 using the SLL/SRA. Meanwhile, it still provides the faster output when SEB/SEH are available.

(I realize I'm violating SSA form in the last three lines of the example but it's just for explanation and should be easy to fix)

269–281

I think we could simplify this to something like:

if (SrcVT.getSizeInBits() <= 16)
  unsigned Mask = APInt::getMaxValue(SrcVT.getSizeInBits()).getZExtValue();
  EmitInst(Mips::ANDi, DestReg).addReg(SrcReg).addImm(Mask);
  return true;
}
return false;

rather than listing each case. It handles unusual types like i9 too.

389–392

Given that trunc is only valid for integers and vectors of integers, do we need to test the type combinations?

I think this this will suffice:

if (SrcVT.isVector() || DestVT.isVector())
  return false;

possibly with a check that the size in bits is <= 32

test/CodeGen/Mips/Fast-ISel/loadstoreconv.ll
1

It would be preferable to write the testcase manually rather than use the output of clang. Clang's output tends to be overly complex and noisy for the kind of tests we need here.

For example:

@b1 = global i8 1, align 1
@i = global i32 0, align 4
define void @sext_i8_to_i16()  {
entry:
  %0 = load i8* @b1, align 1
  %1 = sext i8 %0 to i16
  store i16 %1, i32* @i, align 4
  ret void
}
24

Use CHECK-LABEL so that FileCheck divides the file into independent sections. Likewise for the other function labels below.

rkotler updated this revision to Diff 12369.Aug 11 2014, 3:38 PM
  • d4007
  • Merge branch 'master' into reed-bug-04
  • Merge branch 'master' into reed-bug-04
rkotler abandoned this revision.Aug 11 2014, 3:40 PM

I don't think that any of the proposed changes to my patch are necessary and I don't really agree with them helping the code and to the contrary I think they just make it more complicated and relying on things that may not always be true even, in the future.

Regarding not using the output of clang; this is something i can look into but i'm more comfortable at this time doing it the way I'm doing it and would have to get better at writing LLVM IR by hand.

I would like to submit the patch as is and we can revisit some of this later after the basic Mips fast-isel port is done. A lot of this will change anyway by then and certainly when we add mips64 which is not that far off.

mcrosier added a subscriber: Unknown Object (MLST).Aug 28 2014, 5:33 PM
In D4827#12, @rkotler wrote:

I don't think that any of the proposed changes to my patch are necessary and I don't really agree with them helping the code and to the contrary I think they just make it more complicated and relying on things that may not always be true even, in the future.

I very much agree with Reid.

Regarding not using the output of clang; this is something i can look into but i'm more comfortable at this time doing it the way I'm doing it and would have to get better at writing LLVM IR by hand.

I don't think this is a serious blocker.

I would like to submit the patch as is and we can revisit some of this later after the basic Mips fast-isel port is done. A lot of this will change anyway by then and certainly when we add mips64 which is not that far off.

I'm not giving the LGTM as I haven't closely followed the patch, but I am backing Reid's comments about the code refactoring being unnecessary and in fact harmful.

Chad

mcrosier added inline comments.Aug 28 2014, 5:47 PM
lib/Target/Mips/MipsFastISel.cpp
250

Consider refactoring EmitIntSExt into a single function with a bool parameter, isSigned, that indicates if we're sign-extending or zero-extending. I think as you continue to developer fast-isel you'll see a lot of redundancies between these functions.

356

clang-format?

Comments should be complete sentences with proper punctuation.

391

Pass isZExt as a parameter, per the suggestion above. Logic will be simplified here as well.

410

These are the correct type of checks, IMO. It is much easier to understand when reviewing/refactoring.

In D4827#14, @mcrosier wrote:
In D4827#12, @rkotler wrote:

I don't think that any of the proposed changes to my patch are necessary and I don't really agree with them helping the code and to the contrary I think they just make it more complicated and relying on things that may not always be true even, in the future.

I very much agree with Reid.

I disagree but to a greater or lesser extent according to the change. I'll reply to each one inline with the reason for the request but the overarching principle is that I'd prefer to not to leave gaps in the implementation that need rediscovering at a later point since this has been a major source of pain in both the integrated assembler and code generator. The use of SEB/SEH (which requires MIPS32r2/MIPS64r2) without handling MIPS32r1/MIPS64r1 and earlier is a good example of this.

Regarding not using the output of clang; this is something i can look into but i'm more comfortable at this time doing it the way I'm doing it and would have to get better at writing LLVM IR by hand.

I don't think this is a serious blocker.

Indeed, it's not a serious blocker for an individual patch. I brought it up because I'd prefer that our test cases omit the unnecessary artifacts that clang introduces for its own convenience.

lib/Target/Mips/MipsFastISel.cpp
252–264

The intention of this request is for correctness for multiple ISA's. The code in the original patch is using instructions that only exist on MIPS32r2/MIPS64r2 and above. The request is to support all known MIPS ISA's with a trivial change by falling back on an SHL/SRA sequence.

269–281

This one is refactoring. A 16-bit immediate and instruction is available on all MIPS ISA's and it seemed strange to special case three particular sizes when a single calculation would do the same job.

389–392

This one is a query. It looked like the suggested code was equivalent and would remain so in the future. However, I couldn't be certain.

This initial port is just mips32/r2, pic, O2 . I've added O0 as a requirement. Nothing else is part of what we are doing at this time.

Worrying about other combinations is for future work. Its out of scope at this time, IMO.

SOme of these things, had I know about them, might have made sense for me to do in this first go around but I've already done it this way and this is in the scope of the work and passes all of test-suite so I don't feel there is any need to be making more changes, do the testing, etc. to cases that are out of scope of my assigned work.

In D4827#16, @dsanders wrote:
In D4827#14, @mcrosier wrote:
In D4827#12, @rkotler wrote:

I don't think that any of the proposed changes to my patch are necessary and I don't really agree with them helping the code and to the contrary I think they just make it more complicated and relying on things that may not always be true even, in the future.

I very much agree with Reid.

And now I recant my statement. That's what I get for doing a drive-by review. Thank you for the clarification, Daniel.

I disagree but to a greater or lesser extent according to the change. I'll reply to each one inline with the reason for the request but the overarching principle is that I'd prefer to not to leave gaps in the implementation that need rediscovering at a later point since this has been a major source of pain in both the integrated assembler and code generator. The use of SEB/SEH (which requires MIPS32r2/MIPS64r2) without handling MIPS32r1/MIPS64r1 and earlier is a good example of this.

I didn't realized you were trying to support multiple implementations of the ISA. With that understanding, I'll now back Daniel (strongly). The implementation is incomplete.

Regarding not using the output of clang; this is something i can look into but i'm more comfortable at this time doing it the way I'm doing it and would have to get better at writing LLVM IR by hand.

I don't think this is a serious blocker.

Indeed, it's not a serious blocker for an individual patch. I brought it up because I'd prefer that our test cases omit the unnecessary artifacts that clang introduces for its own convenience.

Very understandable.

What I meant regarding things that might have made sense would be to account for Mips R1 but I really don't see why.

There are lots of Mips target and ABI variations. The scope of my work at this time is just Mips 32/r2, pic, o32 abi.

Everything else is out of scope.

What is in scope is to finish the whole piece of work for this configuration.

Redoing things that are not part of this scope of work slows down the whole project and is definitely contrary to getting the initial work done.

In the constructor for mips fast isel is the following line of code:

TargetSupported = ((Subtarget->getRelocationModel() == Reloc::PIC_) &&
                   (Subtarget->hasMips32r2() && (Subtarget->isABI_O32())));

If TargetSupported is false then we don't do Mips fast isel, even if it is requested.

There is no reason to be concerned with mips32/r1 which is an old processor version as far as I know. We don't even have any build bots at Mips that test if we can pass test-suite with mips32/r1 as the processor as far as I know . If need a special sequence for mips32/r1, then I think it should be on an if statement of some kind rather that use a more obscure sequence that happens to to work on both mips32/r2 and mips32/r1. I used the most clear sequence for mips32/r2 which is our first target for mips fast-isel.

In D4827#18, @mcrosier wrote:
In D4827#16, @dsanders wrote:

And now I recant my statement. That's what I get for doing a drive-by review. Thank you for the clarification, Daniel.

I disagree but to a greater or lesser extent according to the change. I'll reply to each one inline with the reason for the request but the overarching principle is that I'd prefer to not to leave gaps in the implementation that need rediscovering at a later point since this has been a major source of pain in both the integrated assembler and code generator. The use of SEB/SEH (which requires MIPS32r2/MIPS64r2) without handling MIPS32r1/MIPS64r1 and earlier is a good example of this.

I didn't realized you were trying to support multiple implementations of the ISA. With that understanding, I'll now back Daniel (strongly). The implementation is incomplete.

In the long run, yes. Reed is correct that his current work is focused on MIPS32r2 but I also need to look at the long term for the MIPS backend. It has proven difficult to add MIPS1-MIPS5 support on the SelectionDAG side of things because we jumped in at MIPS32r1 and ignored everything that came before and I'd like to avoid repeating the same mistake to a greater degree here. (As an aside, one of my colleagues is working towards fixing some of the MIPS1-MIPS5 issues in the tablegen definitions at the moment).

For easy things such as this case, it seems like an simple call to request the whole implementation. For more difficult things, I'll probably settle for appropriate if-statements and a FIXME comment so we can at least find it again easily.

In D4827#22, @rkotler wrote:

There is no reason to be concerned with mips32/r1 which is an old processor version as far as I know. We don't even have any build bots at Mips that test if we can pass test-suite with mips32/r1 as the processor as far as I know.

We still support MIPS32r1 and we are taking steps to support even earlier processors for various reasons. I'm testing MIPS32r1 as part of the LLVM 3.5 release and it fully passes. It's probably true that there are currently no MIPS32r1 builders at the moment but this is a reflection of lack of facilities rather than lack of need. This is something we're sorting out (along with much more) in the near future.

If need a special sequence for mips32/r1, then I think it should be on an if statement of some kind rather that use a more obscure sequence that happens to to work on both mips32/r2 and mips32/r1. I used the most clear sequence for mips32/r2 which is our first target for mips fast-isel.

Shift-left-shift-right isn't very obscure but more importantly the suggested code continues to use SEB/SEH when they are available so I'm not sure what you are getting at here.

In D4827#21, @rkotler wrote:

In the constructor for mips fast isel is the following line of code:

TargetSupported = ((Subtarget->getRelocationModel() == Reloc::PIC_) &&
                   (Subtarget->hasMips32r2() && (Subtarget->isABI_O32())));

If TargetSupported is false then we don't do Mips fast isel, even if it is requested.

Indeed, this global check reduces the number of configurations you need to test straightaway and I'm ok with it as a way to manage the scale of the early testing. I still have reservations about this check leading to the omission of the local checks in the appropriate places though. If we're doing things correctly then we should be able to delete this check without affecting the correctness of FastISel..

The statement

TargetSupported = ((Subtarget->getRelocationModel() == Reloc::PIC_) &&
                 (Subtarget->hasMips32r2() && (Subtarget->isABI_O32())));

is there because that is the scope of the work I have been assigned by management to do for fast-isel.
There are important customers that need exactly that and nothing more.

It's not there to reduce the testing matrix.

The scope of the work is not to worry about doing fast-isel for other old or new Mips targets or abis.

We may never want to support fast-isel for old targets. That's a decision for management to make as far as fast-isel.
Just because we support old targets in some tools does not mean that it's to be supported uniformly for all new work on llvm, like for fast-isel.

RIght now they just want was is covered by the TargetSupported list.

After that I think the plan is to add 64 bit support.

I don't see how it's difficult to add others in later.

I'm digging my heels in a bit on this patch because I think these kinds of change requests are impeding the task at hand and I'd like
to hopefully rein in the scope here and not have to revisit this issue on every patch; what other things that are beyond the scope of my work assignment
I now to need to take into account and add to my patches.

There are about 10 patches in my queue right now which when submitted will take this first cut at mips fast-isel to a high percentage of completion.