[vector] refactoring api for mask, shuffle creation

[Brian Goetz]

Great.  I sent you some minor API comments yesterday, but otherwise I am happy with where the API has landed.  This is a much more compact and more approachable API than we started with.  

> On Apr 16, 2019, at 8:22 PM, Kharbas, Kishor <kishor.kharbas at intel.com> wrote:
> 
> Hi Brian,
>  
> Switching class Species to an interface does not perturb code generation. In the updated patch I have made this change along with moving the new fields to AbstractSpecies.
>  
> New bug created : https://bugs.openjdk.java.net/browse/JDK-8222584 <https://bugs.openjdk.java.net/browse/JDK-8222584>
> webrev : http://cr.openjdk.java.net/~kkharbas/vector-api/8222584/webrev.01/ <http://cr.openjdk.java.net/~kkharbas/vector-api/8222584/webrev.01/>
>  
> If the patch looks good, I will go ahead and push this.
>  
> Thanks,
> Kishor
>  
> From: Brian Goetz [mailto:brian.goetz at oracle.com] 
> Sent: Monday, April 15, 2019 10:46 AM
> To: Kharbas, Kishor <kishor.kharbas at intel.com>
> Cc: panama-dev at openjdk.java.net; John Rose <john.r.rose at oracle.com>; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
> Subject: Re: [vector] refactoring api for mask, shuffle creation
>  
> This would also be a good time to see if the generated code is perturbed by switching Species to an interface.  Since all the actual Species instances are typed more sharply (e.g., IntSpecies), I think we might be able to get away with this?  If we can do so now, that’s a win, as its one less thing we will be tempted to change later.  
>  
> In any case, the behavior you are adding here (the fields, and the constructor) should be live in AbstractSpecies, where the rest of the state is.  We definitely don’t want to be splitting this across two classes.  
> 
> 
> Thanks Brian for the feedback. I also like the idea of renaming Shape, Species, etc. Code generation is not affected as per my testing so far, I will follow up with more testing.
> Just wanted to make sure changes to Species class (copied below) do not undo the efforts been put to make it value friendly.
>  
>       * @param <E> the boxed element type of this species
>       */
>      public static abstract class Species<E> {
> -        Species() {}
> +
> +        @FunctionalInterface
> +        interface fShuffleFromArray<E> {
> +            Shuffle<E> apply(int[] reorder, int idx);
> +        }
> +
> +        final Function<boolean[], Mask<E>> maskFactory;
> +        final Function<IntUnaryOperator, Shuffle<E>> shuffleFromOpFactory;
> +        final fShuffleFromArray<E> shuffleFromArrayFactory;
> +
> +        Species(Function<boolean[], Vector.Mask<E>> maskFactory,
> +                Function<IntUnaryOperator, Shuffle<E>> shuffleFromOpFactory,
> +                fShuffleFromArray<E> shuffleFromArrayFactory) {
> +            this.maskFactory = maskFactory;
> +            this.shuffleFromOpFactory = shuffleFromOpFactory;
> +            this.shuffleFromArrayFactory = shuffleFromArrayFactory;
> +        }
>          /**
>           * Returns the primitive element type of vectors produced by this
> @@ -1100,6 +1120,18 @@
>              Shape s = Shape.forBitSize(vectorBitSize);
>              return Species.of(c, s);
>          }
> +
> +        interface FOpm {
> +            boolean apply(int i);
> +        }
> +
> +        Vector.Mask<E> opm(Vector.Species.FOpm f) {
> +            boolean[] res = new boolean[length()];
> +            for (int i = 0; i < length(); i++) {
> +                res[i] = f.apply(i);
> +            }
> +            return maskFactory.apply(res);
> +        }
>      }
>  
> Thanks
> Kishor
>  
> From: Brian Goetz [mailto:brian.goetz at oracle.com <mailto:brian.goetz at oracle.com>] 
> Sent: Friday, April 12, 2019 10:18 AM
> To: Kharbas, Kishor <kishor.kharbas at intel.com <mailto:kishor.kharbas at intel.com>>; panama-dev at openjdk.java.net <mailto:panama-dev at openjdk.java.net>
> Cc: John Rose <john.r.rose at oracle.com <mailto:john.r.rose at oracle.com>>; Vladimir Ivanov <vladimir.x.ivanov at oracle.com <mailto:vladimir.x.ivanov at oracle.com>>; Viswanathan, Sandhya <sandhya.viswanathan at intel.com <mailto:sandhya.viswanathan at intel.com>>
> Subject: Re: [vector] refactoring api for mask, shuffle creation
>  
> Moving the classes to top-level is a slam-dunk.  There's no need for nesting in this API.  We might ask ourselves later if we should rename some of these (Shape -> VectorShape) but that's easy if we decide to do so.
> 
> I like the idea of moving the mask/shuffle creation methods to Mask/Shuffle, since they will be easier for developers to find (and secondarily it reduces the surface area of the API.)  Now, the Mask and Shuffle abstractions effectively stand on their own; they are their own top-level entity, and they take control of their own creation.  This results in a simpler API.
> 
> I hope that the generated code remains good with this shift.  
> 
> 
> 
> On 4/11/2019 8:07 PM, Kharbas, Kishor wrote:
> Hi,
> I am experimenting with few changes to improve the API. Specifically,
> 1.       mask and shuffle creation methods (example, maskFromArray()) right now are defined in XxxVector. From user’s perspective it seems more intuitive for Mask/Shuffle to define these methods. This patch moves them out of XxxVector to their respective classes.
> 2.       Moved classes – Species, Shape, Shuffle and Mask, at the top-level instead of being nested inside vector.
>  
> Please help me evaluate the changes. Apart from moving methods around and changing signatures, there are changes in Species class -- addition of factory methods to create masks and shuffles.
> I am in the process of testing the patch.
>  
> Webrev : http://cr.openjdk.java.net/~kkharbas/vector-api/species_refactoring-phase2/webrev-phase2.02/ <http://cr.openjdk.java.net/~kkharbas/vector-api/species_refactoring-phase2/webrev-phase2.02/>
> Javadoc : http://cr.openjdk.java.net/~kkharbas/vector-api/species_refactoring-phase2/webrev_phase-2.JavaDoc.02/jdk.incubator.vector/jdk/incubator/vector/package-summary.html <http://cr.openjdk.java.net/~kkharbas/vector-api/species_refactoring-phase2/webrev_phase-2.JavaDoc.02/jdk.incubator.vector/jdk/incubator/vector/package-summary.html>
>  
> Thanks
> Kishor