> On Apr 20, 2014, at 12:18 PM, Geoffrey Garen <gga...@apple.com> wrote:
>
> Seems like an improvement to me.
>
> Two other things that would improve this code for me:
>
> (1) “Not doing no assertions” hurts my brain. I wish we could use “if
> (ASSERT_ENABLED)” instead.
+1
>
> (2) ASSERT is easier to understand if it only adds code, and does not remove
> code. So, in the “ASSERT else speculateCellI()” code, I’d prefer to make the
> speculateCell() unconditional. That’s easier to read. It’s true that the
> speculateCell() is redundant with the previous ASSERT, but there’s no harm
> done by that.
Unfortunately, the weirdness of the DFG's register allocator means that that
doing a speculateCell and then a SpeculateCellOperand may cause issues. We
expect an operant to be used at most once, and this would sort of look like two
uses.
>
> Geoff
>
>> On Apr 19, 2014, at 1:36 PM, Filip Pizlo <fpi...@apple.com> wrote:
>>
>> Here are some examples.
>>
>> Guarding a loop that does assertions:
>>
>> if (!ASSERT_DISABLED) {
>> for (unsigned index = 0; index < m_generationInfo.size(); ++index) {
>> GenerationInfo& info = m_generationInfo[index];
>> RELEASE_ASSERT(!info.alive());
>> }
>> }
>>
>> The one above comes from DFGSpeculativeJIT.cpp, and it is in a non-trivial
>> method. Using an #if here would obfuscate the control flow and make the
>> method harder to maintain. Arguably, a ASSERT would have been fine because
>> the compiler "should" be smart enough to kill the loop if the ASSERT is a
>> no-op. I'm not sure all compilers would be smart enough, though -
>> particularly if m_generationInfo is any kind of interesting collection class.
>>
>> Guarding code in the JIT that emits an assertion:
>>
>> if (!ASSERT_DISABLED) {
>> JITCompiler::Jump ok = m_jit.branch32(
>> JITCompiler::GreaterThanOrEqual, allocatorGPR, TrustedImm32(0));
>> m_jit.breakpoint();
>> ok.link(&m_jit);
>> }
>>
>> The one above also comes from the DFG JIT. We have lots of code like this.
>> The assertions are very useful but obviously we don't want them in a release
>> build. Using an #if would make the code harder to maintain.
>>
>> Counterexample where we using an #if to guard an assertion in the JIT:
>>
>> #if !ASSERT_DISABLED
>> SpeculateCellOperand op1(this, node->child1());
>> JITCompiler::Jump isOK = m_jit.branchStructurePtr(
>> JITCompiler::Equal,
>> JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()),
>> node->structure());
>> m_jit.breakpoint();
>> isOK.link(&m_jit);
>> #else
>> speculateCell(node->child1());
>> #endif
>>
>> Code like this is harder to understand, especially if the code being guarded
>> is longer than a page. I don't think we should encourage this style, when
>> it would have been clearer to do:
>>
>> if (!ASSERT_DISABLED) {
>> SpeculateCellOperand op1(this, node->child1());
>> JITCompiler::Jump isOK = m_jit.branchStructurePtr(
>> JITCompiler::Equal,
>> JITCompiler::Address(op1.gpr(),
>> JSCell::structureIDOffset()),
>> node->structure());
>> m_jit.breakpoint();
>> isOK.link(&m_jit);
>> } else
>> speculateCell(node->child1());
>>
>> The point of coding conventions is to encourage clarity and common sense. I
>> don't think that #if's improve clarity. I think what I'm proposing is a
>> common sense rule that is easy to follow. I trust everyone to know when a
>> normal if statement would have been semantically equivalent to an #if
>> statement.
>>
>> -Phil
>>
>>
>>> On April 19, 2014 at 1:22:23 PM, Geoffrey Garen (gga...@apple.com) wrote:
>>>
>>> Can you give a motivating example from WebKit?
>>>
>>> Geoff
>>>
>>>> On Apr 19, 2014, at 1:09 PM, Filip Pizlo <fpi...@apple.com> wrote:
>>>>
>>>> Hey everyone,
>>>>
>>>> When guarding code with macros that are always defined, such as
>>>> ASSERT_DISABLED (it's always either 0 or 1), we have a choice between:
>>>>
>>>> if (!ASSERT_DISABLED) {
>>>> // do things
>>>> }
>>>>
>>>> and:
>>>>
>>>> #if !ASSERT_DISABLED
>>>> // do things
>>>> #endif
>>>>
>>>> I'd like to propose that anytime the normal if would be semantically
>>>> equivalent to the preprocessor #if, the normal if should be used.
>>>>
>>>> We don't lose any compiler optimization, since even at -O0, the compiler
>>>> will constant fold the normal if. We do gain a lot of clarity, since the
>>>> control flow of normal if statements is subject to proper indentation.
>>>>
>>>> The "semantically equivalent" requirement still allows for #if to be used
>>>> for thinngs like:
>>>>
>>>> - Guarding the placement of fields in a class.
>>>> - Guarding the definitions of other macros (in the case of
>>>> ASSERT_DISABLED, we define ASSERT in different ways guarded by #if's)
>>>> - Guarding the definition/declaration/inclusion of entire functions,
>>>> classes, and other top-level constructs.
>>>>
>>>> Thoughts?
>>>>
>>>> -Phil
>>>>
>>>> _______________________________________________
>>>> webkit-dev mailing list
>>>> webkit-dev@lists.webkit.org
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
