RFR(M): 8244504: C2: refactor counted loop code in preparation for long counted loop

[John Rose]

On May 25, 2020, at 2:22 AM, Roland Westrelin <rwestrel at redhat.com> wrote:
> 
> 
>> I really like your asserts that
>> test the “fiat type” against the “physics” of the node being
>> created; I mean those that say “type doesn’t match”.
>> Would it make sense to add more such asserts on the
>> other branches?  You’d need a work-alike for the Value
>> method of MaxINode for all the other cases, so (sigh)
>> maybe it’s too much to squeeze into this work.  The
>> lack of good asserts here feels like tech. debt, though.
>> Would you mind filing a suitable bug, if you agree with
>> me?
> 
> Do I understand right that the new bug would be: to provide
> CMoveINode::Value() and CMoveLNode::Value() in the case where the input
> condition follows the pattern of min/max and then add asserts to
> GraphKit::build_min_max(), so essentially the integer interval logic I
> gave up on?

Maybe that’s the right outcome, but I think that interval logic
should be co-located with other MaxNode stuff, rather than
hanging out in move node.cpp.

> 
>> Perhaps the right answer is to move the GraphKit
>> functions (one more time) onto polymorphic factory
>> methods MaxNode::make and MinNode::make, styled
>> like LoadNode::make.
> 
> Sure but there's no MinNode, only a MaxNode:
> 
> class MinINode : public MaxNode {
> 
> Should MaxNode be renamed to MinMaxNode? There's not much in MaxNode so
> I suppose its body could be cloned to introduce a MinNode too.

Kind of like MulNode covers AndNode (IIRC).  The
node supertype factoring is “lumpy” rather than “splitty”.
I think min and max are so closely connected that they
belong in one lump rather than being split apart.

So I think keeping MaxNode as a lump is appropriate here.
If you are comfortable renaming it to MinMaxNode, fine,
but the existing name is acceptable, just like MulNode.
(ExtremumNode feels too pedantic for this code base.)
I’m not totally against splitting out MinNode, either,
but the two classes would surely be coupled behind
the scenes, since the logic is almost the same between
the two.  We wouldn’t want to clone the code and
then reverse all the comparisons, right?  That’s what
boolean flags are for.

The important thing is to keep the min/max logic in
one place, since it is tricky.  I think the new bug should
suggest factoring min/max for I and L so that they
can all be hooked to hardware intrinsics, rather than
boiling them down eagerly to CMoves.  I suggest
(not for now!) allowing min/max nodes to exist
as themselves during GVN passes, and only later
turn to CMoves if the matcher can’t handle them.
(So hardware min/max are optional intrinsics,
to be covered by c-move if they are absent.) 
If that’s a bad idea, and we wish to keep the mix
of MaxI with CMoveL (although it’s awkward)
then we’d want CMoveL to have a little side
channel to talk to to the MaxNode logic when
it can tell it is “really” a MaxLNode.  That’s what
I was thinking.

But for now, the *factory* logic should factor through
MaxNode (or a renamed MinMaxNode), and not be
placed in a random location; my GraphKit suggestion
was wrong.

The rest of your changes are good to go, I think.  Thanks.

— John