Possible typo in src/share/vm/adlc/formssel.cpp:914

[Peter B. Kessler]

I guess that sounds reasonable.  It certainly follows the comment, and explains why you use _components.insert instead of component->promote_use_def_info.

I'm still not clear on why one *inserts* another component with the same name just to add the USE effect, rather than adding that effect to the existing component that has a DEF effect.  For example, I don't think the _components.operand_position(name) just after the outer "if" will ever find the inserted component.  But I'll stare at it some more.

Thanks for the correction.

			... peter

Tom Rodriguez wrote:
> On Jan 25, 2010, at 3:17 PM, Peter B. Kessler wrote:
> 
>> I'm looking in
>>
>>   http://hg.openjdk.java.net/jdk7/jdk7/hotspot/file/3003ddd1d433/src/share/vm/adlc/formssel.cpp
>>
>> at lines 914-915
>>
>>     914       if ((!component->isa( Component::USE) && ((e->_use_def & Component::USE) != 0))) {
>>     915         if (component->isa(Component::USE) && _matrule) {
>>
>> If I have my precedence rules correct, that means I get into the outer then-clause if the component *is not* a USE, but then I never get into the inner then-clause, because it's restricted to a component that *is* a USE.
>>
>> Then there's the funny double-parentheses in the first predicate.  I wonder if line 914 should be
>>
>>     914       if (!(component->isa( Component::USE) && ((e->_use_def & Component::USE) != 0))) {
> 
> Definitely not.  The test should follow the comment logic:
> 
> !       // This happens when adding 'USE' to a component that is not yet one.
> 
> I think the extra parens are just that.  This code was updated as part of 6511991.  I still have the webrev from that and I put it up at http://cr.openjdk.java.net/~never/6511991/src/share/vm/adlc/formssel.cpp.unidiff.html.  I think the parens just got redistributed without removing unneeded ones.  The newly added warning was a copy from above and I think the test wasn't simplified correctly for the context.  I think it should be:
> 
> diff -r cf0685d550f1 src/share/vm/adlc/formssel.cpp                                                                                                         
> --- a/src/share/vm/adlc/formssel.cpp    Wed Jan 20 22:10:33 2010 -0800                                                                                     
> +++ b/src/share/vm/adlc/formssel.cpp    Mon Jan 25 16:23:24 2010 -0800                                                                                     
> @@ -912,7 +912,7 @@ void InstructForm::build_components() {
>        // Check if there is a new effect that requires an extra component.                                                                                 
>        // This happens when adding 'USE' to a component that is not yet one.                                                                               
>        if ((!component->isa( Component::USE) && ((e->_use_def & Component::USE) != 0))) {                                                                   
> -        if (component->isa(Component::USE) && _matrule) {                                                                                                 
> +        if (!component->isa(Component::DEF) && _matrule) {                                                                                                 
>            const Form *form = globalAD->globalNames()[component->_type];                                                                                   
>            assert( form, "component type must be a defined form");                                                                                         
>            OperandForm *op   = form->is_operand();	
> 
> Sound reasonable?
> 
> tom
> 
> 
>> which would get into the outer then-clause if not both the component and the effect were uses, and then would get into the inner then-clause if the component is a USE and there is a match rule (implying that the effect is not a USE).  That seems to make more sense.
>>
>> Is my suggestion right?  Is this worth fixing?
>>
>> 			... peter
>>
>>
>