1125481 - Convert js::jit::FunctionInfo into a variadic template class

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jeff Walden [:Waldo]]

The current macro mess is very difficult to read and debug through, because of its being macros.  Variadic templates would be simpler in some ways.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Non-working patch

I haven't played a ton with variadic templates, but I'm fairly sure this is a substantial start.  But at this point I either hit compiler issues (among our supported compilers, even), standards issues, or my own unwitting bugs -- probably a mix of all of them.  If someone wants to pick this up, feel free.  Dropping it here for that purpose, don't expect to pick it up again.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8554122 [details] [diff] [review]
Non-working patch

Review of attachment 8554122 [details] [diff] [review]:
-----------------------------------------------------------------

"hairy macro tricks"?

I am not yet sure to be able to claim that one approach looks is better than the other.

::: js/src/jit/VMFunctions.h
@@ +470,5 @@
> +// definition.
> +template <template <typename T, size_t Index> class Func,
> +          typename ResultType,
> +          typename... Args>
> +class ApplyAndBitwiseOr

MapReduce<Func, BitwiseOr, ...Args> ?

It sounds that the ResultType is something which can be extracted from BitwiseOR ?

I don't know to which extend templates are capable of expressing something like:
  Reduce<BitwiseOr, Map<Func, List<...Args>>>

this might be handy tools to have in mfb*t*.

@@ +513,5 @@
> +template <template <typename T> class Func,
> +          typename ResultType,
> +          ResultType Default,
> +          typename... Args>
> +class ApplyLast

I think you should split this one in 2, GetLast<...>, and Apply<Fun, T>

  ApplyLast<Func, Default, ...Args> =
    IfEq<GetLast<...Args>::Type, GetLast<>::Type,
         Default,
         Func<GetLast<...Args>::Type> >

Same thing about the ResultType.

@@ +518,5 @@
> +{
> +    template <typename... Rest>
> +    struct Apply
> +    {
> +        static const ResultType value = Func<typename LastType<Args...>::Type>::value;

nit: s/Args.../Rest.../ ?

Comment 3

[Jeff Walden [:Waldo]]

Yeah, there are likely/probably some tactics in there that could be extracted into general utilities.  In advance of much variadic template use on my part, I was hesitant to try very hard on that front.

Honestly not gonna think too hard about the other parts of this right now, given lurking compiler-bug suspicions, and other higher-priority issues to solve.  If anyone who's actually used variadic templates before much wants to chime in, our nation will turn its lonely eyes to you!  (...whoohoohooooo...)

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Replace FunctionInfo macros by variadic templates.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8610813 [details] [diff] [review]
Replace FunctionInfo macros by variadic templates.

Review of attachment 8610813 [details] [diff] [review]:
-----------------------------------------------------------------

My eyes glazed over some, but I *think* this is okay.

::: js/src/jit/VMFunctions.h
@@ +448,5 @@
> +struct LastArg<>
> +{
> +    typedef void Type;
> +    enum { index = -1 };
> +};

Can you just leave this empty (or even not define it at all, just declare it) so that accidental use of this is a compile error?  LastArg on a non-empty list seems categorically wrong to me.

@@ +454,5 @@
> +template <typename ArgType>
> +struct LastArg<ArgType>
> +{
> +    typedef ArgType Type;
> +    enum { index = 0 };

Make this a static MOZ_CONSTEXPR_VAR size_t, please.

@@ +458,5 @@
> +    enum { index = 0 };
> +};
> +
> +template <typename ArgType, typename... ArgTypes>
> +struct LastArg<ArgType, ArgTypes...>

Again, HeadType and TailTypes.

@@ +461,5 @@
> +template <typename ArgType, typename... ArgTypes>
> +struct LastArg<ArgType, ArgTypes...>
> +{
> +    typedef typename LastArg<ArgTypes...>::Type Type;
> +    enum { index = LastArg<ArgTypes...>::index + 1 };

And of course static MOZ_CONSTEXPR_VAR size_t here, too.

@@ +467,5 @@
>  
> +// Construct a bit mask from a list of types.  The mask is constructed as an OR
> +// of the mask produced for each argument. The result of each argument is
> +// shifted by its index, such that the result of the first argument is on the
> +// low bits of the mask, and the result of the latest argument in part of the

s/latest/last/

@@ +469,5 @@
> +// of the mask produced for each argument. The result of each argument is
> +// shifted by its index, such that the result of the first argument is on the
> +// low bits of the mask, and the result of the latest argument in part of the
> +// high bits of the mask.
> +template <template<typename> class Each, typename ResultType, size_t shift,

Capitalize |shift| as a template parameter, please.

@@ +476,5 @@
>  
> +template <template<typename> class Each, typename ResultType, size_t shift>
> +struct BitMask<Each, ResultType, shift>
> +{
> +    static const ResultType result = ResultType();

MOZ_CONSTEXPR_VAR

@@ +481,4 @@
>  };
>  
> +template <template<typename> class Each, typename ResultType, size_t shift,
> +          typename Arg, typename... Args>

typename HeadArg, typename... TailArgs

@@ +483,5 @@
> +template <template<typename> class Each, typename ResultType, size_t shift,
> +          typename Arg, typename... Args>
> +struct BitMask<Each, ResultType, shift, Arg, Args...>
> +{
> +    static_assert(Each<Arg>::result < (1 << shift),

ResultType(1) in there, please -- this would start being sad if someone used a ResultType of uint64_t around 32 arguments, for no good reason.

@@ +486,5 @@
> +{
> +    static_assert(Each<Arg>::result < (1 << shift),
> +                  "Not enough bits reserved by the shift for individual results");
> +    static_assert(LastArg<Args...>::index + 1 < (8 * sizeof(ResultType) / shift),
> +                  "Not enough bits in the result type to store all bit masks");

Lowercase the starts of these reasons, please -- seems like compilers generally place these in a lowercased context.

@@ +488,5 @@
> +                  "Not enough bits reserved by the shift for individual results");
> +    static_assert(LastArg<Args...>::index + 1 < (8 * sizeof(ResultType) / shift),
> +                  "Not enough bits in the result type to store all bit masks");
> +
> +    static const ResultType result =

MOZ_CONSTEXPR_VAR

@@ +500,5 @@
> +template <typename... Args>
> +struct FunctionInfo;
> +
> +template <class R, class Context, typename... Args>
> +struct FunctionInfo<R (*)(Context, Args...)> : public VMFunction {

{ on fresh line

@@ +506,2 @@
>  
>      static inline DataType returnType() {

In a separate patch, could you remove all the inlines on these methods?  Methods defined inline in a class are already inline, this is just clutter.  r=me on that.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> ::: js/src/jit/VMFunctions.h
> @@ +448,5 @@
> > +struct LastArg<>
> > +{
> > +    typedef void Type;
> > +    enum { index = -1 };
> > +};
> 
> Can you just leave this empty (or even not define it at all, just declare
> it) so that accidental use of this is a compile error?  LastArg on a
> non-empty list seems categorically wrong to me.

This case is actually needed, so I renamed |index| to |nbArgs| and added 1.

Comment 7

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5b2f1873144

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/803a7913c115
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb2e7d00e46

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/803a7913c115
https://hg.mozilla.org/mozilla-central/rev/fbb2e7d00e46