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

[Tom Rodriguez]

The components list has multiple functions.  A primary one is that it functions as a mapping between components and the index of the input in the resulting node.  The DEF is always at 0 and any component that corresponds to a true node comes next in sequence.  So tacking USE onto the DEF without adding a new component won't do the right thing.  Components like KILLs of the flags don't correspond to true inputs.  The adlc design is a bit of a mess and could use a significant rewrite but given the state of the code that would likely entail introducing new bugs.

tom

On Jan 25, 2010, at 5:31 PM, Peter B. Kessler wrote:

> 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
>>> 
>>> 
>