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

[Tom Rodriguez]

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