Patch updates per review remarks

@jdenny ? @sepavloff ?
ping

Hi Pete, I'll go on vocation from next week. And will offline till 10th of March. If you have some diffs to show, please don't hesitate for too long ;-)

Hello Pete,
I've looked at this patch. You add support for dynamic GEPs. I was on non-llvm project for a while, and dynamic GEP is quite new thing, at least when you can use arrays as indices. It also a bit strange, that we can use only splat arrays.. Anyways, I suppose we should get vector of pointers in this case, right? Can you provide me with links, where I can read more about?

Hi Pete,
Thanks for patch. The idea looks good, but I'm still in middle of review..

Hi Janne,
What exactly you'd like to merge? Did mean to use GNU style (non-Darwin) everywhere?

Committed as r213060
Thanks for working on it!

ping
Stepan Dyatkovskiy wrote:

Hi Nick,
can I commit patch for Björn?
Thanks!
-Stepan

Stepan Dyatkovskiy wrote:

LGTM

http://reviews.llvm.org/D4246

Hi Nick,
can I commit patch for Björn?
Thanks!
-Stepan

Andrey,
do you need us to commit it?

LGTM,
now let's wait Nick and talk to him :-)

Changes in MergeFunctions.cpp - LGTM
call-with-ranges.ll - LGTM
invoke-with-ranges.ll - read inline comment, otherwise - fine.

Yup, Renato proposed good idea. Final patch looks fine, except tiny thing, could you use single drop_front call (see inline comments)?

Hi Björn,

Hi Andrey,
Thanks for working on it!
I think we have to keep compatibility with GAS.. So in general patch looks reasonable. May be it would be good to add some details what method does (see inline comments).
I've CCed Renato and Tim for approval.

Committed as r211391.
Thanks!

Hi Nick,
Can I commit it for Björn?
Thanks!
-Stepan

Hi Bjorn,
Do you need us to commit your patch? I could do it, if Nick won't object.

LGTM! Thanks!

Yes. Exactly ) For now, I propose to come back to initial patch; make sure tests has been fixed as I asked you in inline comments and, I think we apply it (Nick didn't object about it yesterday).
Then, after we replace ComparableFunction with new structure, you could introduce 'range' metadata merging.
Thanks for working on it!

Hi Bjorn (sorry I don't have o-umlaut on my keyboard :-) ),

This test has been committed with some changes as r210486. Thanks!

Hi Bjorn,
You have found really interesting point :-)
For this changes set, please, read inline comments.

OK. I'll try to commit it for you in nearest days. All I need is meet Nick in IRC and ask about this test.

Hi Bjorn,
We have replaced "enumerate" method with "cmpValues" + "cmpConstants". That was made as part of bigger improvement.
Now getBitcast is not called. But your test is really usefull.
So I would like to ask you try your test with trunk llvm version. I think it should work now. If so, I try to organize commit for you today.

So guys, how do you find this fix?

I have the same opinion. IMHO, the best solution is to extend Db with information about what was parsed. But how we could solve it now? Is there any other ways to distinguish operator> from tail of "<some_operator>" ?

r207080
Thanks for reviews!

r206951
Thanks for reviews!

Its good to me! Sean, what do you think?

I have talked with Chandler in IRC. My last suggestion contradicts LLVM design, because constants should never been removed.
So, that means we better stay on your current solution. I would be glad if you fix the tests anyway ;-)
Thanks!

So, getBitCast creates new Value (call it C3).
C3 is not equal to C1, but it still hangs in air.
Perhaps then just as another solution, we could do the next:

1. Check for bitcast possibility
2. Create bitcast (C3)
3. Check whether C1 is equal to C3
4. Get rid of C3.

Yes, it would be good if you increase test size. So we could be sure it will run whole the comparison routines.

It doesn't return true.

Björn,
I think the fix of canLosslesslyBitcasted could be only accepted in case, when functions are not bitcastable at all. But I think they are. At least you can define such bitcast in globals. So I think, we better do our best from MergeFunctions side, keep it stable, file a bug, so then we could work on it afterwards.

Dotdash, thank you for working on it!