1225908 - Disambiguate SimdTypeToScalarType

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

Ion has two functions called SimdTypeToScalarType with similar purpose:

In IonTypes.h:
  static inline MIRType
  SimdTypeToScalarType(MIRType type)

In MCallOptimize.cpp:
  static Scalar::Type
  SimdTypeToScalarType(SimdTypeDescr::Type type)

We should rename both of these to make their purpose clearer.

Maybe add some documentation too?

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

Uack, Scalar::Type actually includes Int32x4 and Float32x4.
Maybe we shouldn't refer to vector lanes as 'scalars' to avoid confusion.

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Attachment: SimdTypeToLaneType.

Rename the function SimdTypeToScalarType to SimdTypeToLaneType. Since we also
have Scalar::Type, it is best to use the term 'lane' to refer to the parts of a
vector.

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Attachment: SimdTypeToArrayElementType.

This function returns the typed array element type that corresponds to the
lanes in a SIMD vector type. It is used to handle SIMD loads and stores from a
typed array.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Ben, we could also rename s/AsmSimdTypeToScalarType/AsmSimdTypeToLaneType/. What do you think?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8689122 [details] [diff] [review]
SimdTypeToLaneType.

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

That's better, thank you for the patch.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8689123 [details] [diff] [review]
SimdTypeToArrayElementType.

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

Yes, that's indeed better.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #4)
> Ben, we could also rename s/AsmSimdTypeToScalarType/AsmSimdTypeToLaneType/.
> What do you think?

That would work for me. We've used Scalar at several locations to refer to the lane type (in opposition to the vector type), but saying Lane is clearer, in my opinion.

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

Attachment: AsmSimdTypeToLaneType.

Rename AsmSimdTypeToScalarType to AsmSimdTypeToLaneType, hoping that the new
name will be clearer.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8689589 [details] [diff] [review]
AsmSimdTypeToLaneType.

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

Thanks!

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94d80c1508bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9a544bc43e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d63e7bf319f4

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94d80c1508bf
https://hg.mozilla.org/mozilla-central/rev/1e9a544bc43e
https://hg.mozilla.org/mozilla-central/rev/d63e7bf319f4