[llvm-commits] mips commit regarding RA register problem

[Kotler, Reed]

I'd like to just leave this patch alone. 
You are okay if we add a comment to it?

I tried some quick solutions for making the patch cleaner but it caused just the very problems that led me to implement the current patch the way it is.

The problem here is way more involved than I want to solve right now. I'm trying to finishing debugging and put back the whole rest of the mips 16 port and this is not going to make any difference for now.

The problem is that RA is not a mips 16 register. It can only be easily saved and restored with the save/restore instruction. For now I'm just always saving and restoring it, even if it's not used. If it's not used at all, then probably I can avoid adjusting the stack size in the prologue and epilogue code.

i don't want to tell the rest of LLVM about it because it just creates problems.

If I tell it about the register then it often wants to save or restore it to the stack and this cannot be done easily except with save/restore and there is no reason to even do that operation except with save/restore on function entry/exit.

Probably if I study the ARM code I will see a simple way to do all of this but it's just an optimization for now and not relevant to me for getting a stable base port checked in.

We do something else in mips 32 but once again the issue is that restoring the register needs to be done by the restore instruction and not by the normal mechanism for loading from the stack because there is no such instruction in mips 16. 

________________________________________
From: Kotler, Reed
Sent: Monday, September 17, 2012 6:29 PM
To: Bill Wendling
Cc: llvm-commits at cs.uiuc.edu
Subject: RE: [llvm-commits] mips commit regarding RA register problem

It's not just a question of making changes but time to test, etc.


________________________________________
From: Kotler, Reed
Sent: Monday, September 17, 2012 6:27 PM
To: Bill Wendling
Cc: llvm-commits at cs.uiuc.edu
Subject: RE: [llvm-commits] mips commit regarding RA register problem

We are making a change to fix this problem which will revert the patch. I already put in the original change, comments saying that it's a temporary fix and the string TBD, etc.

There are some deep issues at play here that I don't have time to go into. I was not simply hacking.

In the interest of making everyone happy,we have made a clean solution to this which although clean, will be thrown away when I do the full implementation of save/restore instructions for Mips 16. That is why I did what I did; because I did not want to waste time implementing things that are going to be deleted in a month.

Reed



________________________________________
From: Kotler, Reed
Sent: Monday, September 17, 2012 5:40 PM
To: Bill Wendling
Cc: llvm-commits at cs.uiuc.edu
Subject: RE: [llvm-commits] mips commit regarding RA register problem

We can revert this and fix getStackSize right now if that is what is the problem.

I already commented that it's a hack and temporary and put the words TBD in there.

________________________________________
From: Kotler, Reed
Sent: Monday, September 17, 2012 5:05 PM
To: Bill Wendling
Cc: llvm-commits at cs.uiuc.edu
Subject: RE: [llvm-commits] mips commit regarding RA register problem

I have the whole rest of the mips 16 port sitting in our local repository.

I can't check it in all at once because it's too big.

So I need to check in all the pieces and then I can do more  work. This is what I'm trying to do right now.

What part of this check in is bothering you?

This code will work forever. It's correct.

What you complained about is the test for a 0 stack size, which  cannot happen right now because
I am forcing it to have a non zero stack size.

I don't want to delete that code because it will become allowable once again when I do the optimization which makes it possible to not save RA at all.

We CANNOT revert this patch right now and continue our work.

The code we are checking in works. Why is it a horrible hack? It is correct. it will work until the Sun runs out of fuel and stops heating the planet.

You want us to delete two lines of code that test that stack size is 0?

What problem can this cause?

There are NO bugs caused by this checkin.

The horrible bug is what happens without this checkin.

Reed


_______________________________________
From: Bill Wendling [wendling at apple.com]
Sent: Monday, September 17, 2012 4:37 PM
To: Kotler, Reed
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] mips commit regarding RA register problem

On Sep 17, 2012, at 3:40 PM, "Kotler, Reed" <rkotler at mips.com> wrote:

> This patch I submitted is needed to fix RA is temporary in a relative sense, it will be there for a few weeks until I have time to add the code so that saving RA is optional for Mips.
>
> When I make that patch, then the following line which checks for the stack size being 0 will be relevant again.
>
> This was a matter of optimizations and changes occurring in mips32 and mips64 that are not possible in Mips 16 at this time.
>
> The patch is needed or else many tests will fail.
>
> I'm in the process of trying to pass all the tests in test-suite and this bug was found.
>
> Nobody is using the mips 16 port now because it is not all checked in even.

This sounds like something that's affecting you locally. Why not have the hack in your checked out version of the compiler instead of putting it into the main trunk?

> I'm in the process of checking in the code.

Good. However, that doesn't address the important point here. We try *very* hard to keep the trunk as stable and bug-free as possible. This implies that hacks to work around problems are highly discouraged. Not only is it bad programming, but they might be missed and stay in the code base. Or worse, someone could come along and write code that depends upon the hack. It matters not whether this applies to this situation.

I *really* would prefer you revert the patch and keep the hack local to your code until you're ready to check in the correct code. If you refuse, then you need to at least comment that awful hack to make sure that no one relies upon it.

-bw