[llvm-commits] [PATCH 07/20] [AVX] Unique BitInit

[David Blaikie]

On Tue, Jul 19, 2011 at 1:32 PM, David A. Greene <greened at obbligato.org> wrote:
> David Blaikie <dblaikie at gmail.com> writes:
>
>>> -  return new BitInit(V);
>>> +  static const BitInit *True  = 0;
>>> +  static const BitInit *False = 0;
>>> +
>>> +  const BitInit **Result = (V ? &True : &False);
>>> +
>>> +  if (*Result == 0)
>>> +    *Result = new BitInit(V);
>>> +
>>> +  return *Result;
>>
>> You could use a reference here:
>>
>> const BitInit &*Result = (V ? True : False);
>> if (Result == 0)
>>   Result = new BitINit(...);
>>
>> I'm not sure what LLVM's coding conventions are on the use of
>> immediate static values rather than lazy initialization, but in this
>> case is there a particular reason these are lazy? (if so, a comment
>> explaining the necessity, if it's out of character/special, might be
>> in order)
>
> I'm not sure what you mean by "lazy."  Oh, you mean why aren't they
> initialized until needed?  I suppose there is no good reason.  I
> can certainly rework that.

Yeah, just doing:
static const BitInit True(true);
static const BitInit False(false);
return V ? True : False;

I'd probably put these constants outside the function, just so the
compiler doesn't emit its own lazy initializers (if it can prove that
the construction order is irrelevant, then it could skip the lazy
init, but otherwise it must init these local statics on the first
call, not before)

> As for references, this is the old C guy in me creeping out.  I try to
> keep him down as much as possible but he was too wily the other day.  :)

Yep, it happens - and in this case it's perhaps not strictly better
(eh, it's a non-issue if you're removing the manual lazy init anyway),
I was just reminded of another CR last week or the week before when
someone went to fix some code that looked like it was broken
(retrieved a reference to a pointer from a map, checked the pointer,
inited the pointer if necessary, then did nothing else - the reader
thought that the value hadn't been put back in the map, but it had via
the reference) so it's certainly not the most obvious code, but in
your case the scope of the reference would've been shorter (& thus
more obvious) than in the other one I'm thinking of.

Of course this lazy init feedback applies to your other CRs too -
though I think only one other Init type has lazy init of a known set
of values (the UnInit type, or similar).

- David