[cfe-commits] r65746 - in /cfe/trunk: lib/CodeGen/CGBlocks.cpp lib/CodeGen/CGExprConstant.cpp lib/CodeGen/CodeGenFunction.h test/CodeGen/blocks.c

[Mike Stump]

On Mar 1, 2009, at 12:28 PM, Anders Carlsson wrote:
> On Mar 1, 2009, at 11:12 AM, Mike Stump wrote:
>
>> On Mar 1, 2009, at 10:58 AM, Anders Carlsson wrote:
>>> 1 mar 2009 kl. 10.51 skrev Mike Stump:
>>>
>>>> On Feb 28, 2009, at 5:09 PM, Anders Carlsson wrote:
>>>>> +  // Check if the block can be global.
>>>>> +  if (CanBlockBeGlobal(Info))
>>>>> +    return CGM.GetAddrOfGlobalBlock(BE, Name.c_str());
>>>>
>>>> Like the last change, this also breaks things that were  
>>>> working.  :-(  Please, monotonic improvements only please.  We  
>>>> lazily create imports during codegen and your code doesn't  
>>>> account for that.
>>>
>>> I think it is better to just use one code path for creating global  
>>> blocks.
>>
>> Why?  It replicates the other code entirely.
>
> If one code path replicates the other code path, then why have two  
> code paths in the first place?

Because I've not had a chance to rip your code out.  :-)  There is one  
little thing your code path does, that mine, doesn't do yet, and that  
is to have fewer descriptors in the generated code.  I hate to regress  
that one feature, which is probably why I've not done that yet.

> I think at this stage it's simpler to look for the block variables  
> beforehand

I think correctness, good design and speed should trump simplicity.   
During code generation, we must figure out all these little details.   
So, figuring out that details twice, is silly, and, poor design, as we  
have to then maintain the checking code, and the generation code.  By  
having the checking code in the natural place, it would have worked  
the first time and not have had the regression, and it would have been  
easier to spot the fact there is codegen support for __block.  I do  
like the checking code for the unsupported things, just, it needs to  
buried down inside the code that does the walking to begin with.

> - it simplifies the "Unsupported" checks

I don't think this is the case.  I think the proper code is smaller  
than your version and is simpler as well.

> and it also makes code like
>
> void f() {
> 	int a;
>
> 	^{
> 		^{
> 			a;
> 		};
> 	};
> }
>
> easier to deal with.

By easier, do you mean getting the check wrong and having to spend  
time discussing it and fixing it?  I don't agree that is easier.

> In the future we could even be clever and sort imported block  
> variables by size/alignment to minimize memory wasted in "holes".

Orthogonal.  I sort by appearance order to minimize cache use and  
misses.  :-)  Though, I agree, there is room for more intelligent  
packing.

>>> I added code to look up imports beforehand.
>>
>> And my point it, is is wrong.  It is slow, I don't want to walk  
>> around looking for all the data you walk around for and it is  
>> unnecessary and you broke previously working code like:
>>
>
> Traversing isn't that slow...

It is slower than the well designed checking code that is simpler,  
easier to get right and smaller.  I don't see a compelling reason to  
make it slow.  Worse though, it violates layering, and it is that  
violation that leads to the incorrect error.

> we could even make it only traverse blocks that we know have  
> variables.

And that is yet one more step in the wrong direction.

> (If by "broke" you mean that it now gives an "unsupported" error,  
> then yes I broke it ;)

You say that like producing hard errors for code that is valid and  
previously works is somehow not that bad.  Imagine we're trying to QA  
projects that used to build that now don't build.