Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Devel regression on static semcheck #10339

Closed
mratsim opened this issue Jan 16, 2019 · 0 comments

Comments

Projects
None yet
1 participant
@mratsim
Copy link
Collaborator

commented Jan 16, 2019

The commit 87f8ec5 introduces a regression in my Laser library.

I get ../../laser/primitives/matrix_multiplication/gemm_tiling.nim(123, 6) Error: object constructor needs an object type

At the function x86_ukernel with signature:

type
  MicroKernel* = object
    mr*, nr*: int
    cpu_simd*: CPUFeatureX86
    nb_scalars*: int # Ideally MicroKernel should be generic over T, but static generic ...
    nb_vecs_nr*: int
    c_unit_stride*: bool

  CPUFeatureX86* = enum
    x86_Generic,
    x86_SSE,
    x86_SSE2,
    x86_SSE4_1,
    x86_AVX,
    x86_AVX_FMA,
    x86_AVX2,

func x86_ukernel*(cpu: CPUFeatureX86, T: typedesc, c_unit_stride: bool): MicroKernel =

Unfortunately this is quite a tricky bug so the minimal repro is to clone the repo and use the following compilation command:

../Nim/bin/nim c -d:release -d:openmp -o:build/gemm_i32_omp benchmarks/gemm/gemm_bench_int32.nim

Everything of interest is happening in this folder https://github.com/numforge/laser/tree/master/laser/primitives/matrix_multiplication, especially the

  • gemm.nim
  • gemm_packing.nim
  • gemm_tiling.nim

The full trace is:

gemm_bench_int32.nim(121, 8) template/generic instantiation of `bench` from here
gemm_bench_int32.nim(126, 17) template/generic instantiation of `gemm_strided` from here
../../laser/primitives/matrix_multiplication/gemm.nim(211, 46) template/generic instantiation of `dispatch` from here
../../laser/primitives/matrix_multiplication/gemm.nim(194, 14) template/generic instantiation of `apply` from here
../../laser/primitives/matrix_multiplication/gemm.nim(185, 30) template/generic instantiation of `gemm_impl` from here
../../laser/primitives/matrix_multiplication/gemm.nim(126, 29) template/generic instantiation of `pack_B_kc_nc` from here
../../laser/primitives/matrix_multiplication/gemm_tiling.nim(123, 6) Error: object constructor needs an object type

Important remarks

When implementing Laser, I had the error a lot at the beginning, when I tried to use the following function signature for "packing functions":

proc pack_B_kc_nc*[T](
      ukernel: static MicroKernel,
      packedB: ptr UncheckedArray[T],
      kc, nc: int,
      B: MatrixView[T])

# instead of

proc pack_B_kc_nc*[T; ukernel: static MicroKernel](
      packedB: ptr UncheckedArray[T],
      kc, nc: int,
      B: MatrixView[T]) =

This PR/branch numforge/laser#16 shows that behaviour when passing static object as parameter instead of as generic param except for that case it never worked.

Furthermore, when replacing the function result type by auto in

func x86_ukernel*(cpu: CPUFeatureX86, T: typedesc, c_unit_stride: bool): MicroKernel =

I narrowed the issue to the static instantiation of Microkernel.

cc @zah

Vindaar added a commit to Vindaar/Nim that referenced this issue Jan 18, 2019

Vindaar added a commit to Vindaar/Nim that referenced this issue Jan 21, 2019

Vindaar added a commit to Vindaar/Nim that referenced this issue Jan 21, 2019

revert check for nkObjConstr, return type from nkEmpty node
The correct type needed in `semObjConstr` to fix nim-lang#10339 is indeed
available, but attached to an `nkEmpty` node. These were previously
discarded in `semTypeNode`, which is used to extract the type for the
object.

Vindaar added a commit to Vindaar/Nim that referenced this issue Jan 21, 2019

Vindaar added a commit to Vindaar/Nim that referenced this issue Jan 21, 2019

revert check for nkObjConstr, return type from nkEmpty node
The correct type needed in `semObjConstr` to fix nim-lang#10339 is indeed
available, but attached to an `nkEmpty` node. These were previously
discarded in `semTypeNode`, which is used to extract the type for the
object.

@Araq Araq closed this in d9ee377 Jan 23, 2019

ThomasTJdev added a commit to ThomasTJdev/Nim that referenced this issue Jan 27, 2019

fix nim-lang#10339 by returning type attached to nkEmpty (nim-lang#10370
)

* fix nim-lang#10339 by checking for nkObjConstr

* revert check for nkObjConstr, return type from nkEmpty node

The correct type needed in `semObjConstr` to fix nim-lang#10339 is indeed
available, but attached to an `nkEmpty` node. These were previously
discarded in `semTypeNode`, which is used to extract the type for the
object.

* simplify return of PType from  `nkEmpty`

* also fixes nim-lang#9866, add test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.