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

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Waldo, Assigned: nbp)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8554122 [details] [diff] [review]
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 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.../ ?
(Reporter)

Comment 3

3 years ago
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...)
Created attachment 8610813 [details] [diff] [review]
Replace FunctionInfo macros by variadic templates.
Attachment #8610813 - Flags: review?(jwalden+bmo)
Assignee: jwalden+bmo → nicolas.b.pierron
(Reporter)

Comment 5

3 years ago
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.
Attachment #8610813 - Flags: review?(jwalden+bmo) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/803a7913c115
https://hg.mozilla.org/mozilla-central/rev/fbb2e7d00e46
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.