[PATCH] Add sign/zero extend to mips fast-isel

[Daniel Sanders]

>>! 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..

http://reviews.llvm.org/D4827