[llvm] r181753 - This is the first of three patches which creates stubs used for

[reed kotler]

On 05/15/2013 10:28 AM, Eric Christopher wrote:> On Tue, May 14, 2013 at 
8:21 PM, reed kotler <rkotler-8NJIiSa5LzA at public.gmane.org> wrote:
 >> Hi Jim,
 >>
 >> Doug Gilmore is the manager here and he sent his view.
 >>
 >
 > Not sure that matters to anyone but you.
 >
 >> My view is that some people don't like the idea of doing inline 
assembly and
 >> I don't see any issue with it. I did not feel there were any strong
 >> objections and neither Rafael, Eric C. nor Hal complained about this
 >> checkin. It's subjective and I don't see how to settle it.
 >>
 >
 > I'll happily complain. I think this is the wrong way to do it. Rafael
 > also listed some other objections.
 >
 >> This pass is a self contained module pass which solves a very messy 
problem
 >> in mips16 in a very clear way. There are many cases to this and pic and
 >> static have some very different issues and there are endian issues 
too. In
 >> addition I have to change instruction sets because the code is being
 >> compiled for mips16 but the stubs are mips32 code.
 >
 > I'm not sure that this is an issue. Why is this a problem for you?
 >
It's not a problem but it was very clear and easy to do in IR and I 
cannot say it would be like that later on in the compilation path.

 >>
 >> I want to just finish checking in the solution.
 >>
 >> After I do, if someone wants to look at the totality of the problem and
 >> offer a better solution, I'm open to making changes. I think it's 
unfair to
 >> insist there is a better way without looking
 >> at the whole problem.
 >>
 >
 > Why not submit the whole patch for review first then?
 >

People are told not to submit large patches so I did not even consider 
that option.

I could have but the whole patch is probably 1000 lines. Much of it was 
checked in with the return helpers patch which did not use stubs.

I tried to break it up into pieces that were functionally separable that 
could easily be understood. I also took time to clean up code and Akira 
reviewed the smaller patches.

Almost everything is already checked in.

The next patch is just one small function of maybe 40 lines. I've 
already factored a lot of the code so this routine can use common helper 
functions.

The last patch will be small. I forget to make the stub for the case of 
function calls through ptr dereference. It will be a small patch but I 
chose to just do it after everything else is checked in and tested. It 
will be using common code already checked in for all the heavy lifting.

 >> I did a very simple part of this problem using selection dag earlier 
and it
 >> was messy. I've said before that maybe someone that is more 
comfortable with
 >> DAG transformation would offer a better solution.
 >>
 >> This was in  r173320
 >
 > I saw that. There's also MI lowering, which could be easier as I said.
 >
I'm not against moving some things later as the smoke clears.
You can advise when you see the whole patch. I don't profess to be any 
kind of LLVM expert. I seek advice often on the list.

 > It may be an aesthetic choice, but given how many people are saying
 > that your aesthetics are wrong you may wish to reconsider your
 > position.
 >
 > Ultimately if it is really going to touch nothing in any other pass or
 > backend and I don't have to maintain it then I'm not going to put up
 > too much of a fuss.
 >
 > One specific about your testing though, if this is going to be a
 > module pass you should run your test just the input/output of the pass
 > and not the entire backend. Please provide tests for this.
 >
Is it necessary to do this other kind of alternate testing right now 
before checking in the rest?

I already have tests for checking that the assembly code is correct in 
the stubs. That is what I'm generating and that is what I want to verify.

Can I file a fixme bug against myself for this?

If we do decide to move the inline assembly later in the compilation 
path, then the testing for assembly would still apply but not for just 
testing IR.

It would be possible dump the IR I guess and look at it.
Is this what you are asking?

I have not done that kind of test in past for LLVM.


Thanks for taking the time to make such a detailed response.

I'm always learning.

Reed