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

IC: green tests #17311

Merged
merged 19 commits into from
Mar 19, 2021
Merged

IC: green tests #17311

merged 19 commits into from
Mar 19, 2021

Conversation

Araq
Copy link
Member

@Araq Araq commented Mar 9, 2021

To do:

  • fix arraymancer regression
  • make tgeneric test case green
  • write a method test case (make the methods .inline for good measure)
  • EDIT(timotheecour) figure out how to un-disable tests/dll/nimhcr_integration.nim

@Araq Araq changed the title IC: renamed to_packed_ast module to ic module IC: green tests Mar 12, 2021
@Araq Araq merged commit 6c1c8f5 into devel Mar 19, 2021
@Araq Araq deleted the araq-ic19 branch March 19, 2021 15:53
ringabout pushed a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
* IC: renamed to_packed_ast module to ic module

* IC: don't store the --forceBuild flag, makes it easier to test

* IC: enable hello world test

* Codegen: refactorings for IC; changed the name mangling algorithm

* fixed the HCR regressions

* life is too short for HCR

* tconvexhull is now allowed to use deepCopy

* IC exposed a stdlib bug, required a refactoring

* codegen: code cleanups

* IC: even if a module is outdated, its dependencies might come from disk

* IC: progress

* IC: better name mangling, module IDs are not stable

* IC: another refactoring helping with --ic:on --gc:arc

* disable arraymancer on Windows for the time being

* disable arraymancer altogether

* IC: make basic test work with 'nim cpp'

* IC: progress on --ic:on --gc:arc

* wip; name mangling for type info
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* IC: renamed to_packed_ast module to ic module

* IC: don't store the --forceBuild flag, makes it easier to test

* IC: enable hello world test

* Codegen: refactorings for IC; changed the name mangling algorithm

* fixed the HCR regressions

* life is too short for HCR

* tconvexhull is now allowed to use deepCopy

* IC exposed a stdlib bug, required a refactoring

* codegen: code cleanups

* IC: even if a module is outdated, its dependencies might come from disk

* IC: progress

* IC: better name mangling, module IDs are not stable

* IC: another refactoring helping with --ic:on --gc:arc

* disable arraymancer on Windows for the time being

* disable arraymancer altogether

* IC: make basic test work with 'nim cpp'

* IC: progress on --ic:on --gc:arc

* wip; name mangling for type info
@Vindaar
Copy link
Contributor

Vindaar commented Jan 8, 2022

@Araq The arraymancer regression you mention is:

mratsim/Arraymancer#505

With the additional comments and the likely related issue status-im/nim-blscurve#133 any ideas on a fix?
For arraymancer we've introduced a workaround, but it seems to be a bigger issue.

@Araq
Copy link
Member Author

Araq commented Jan 8, 2022

The other comparable problems were all simply serious bugs in the C code.

@mratsim
Copy link
Collaborator

mratsim commented Jan 8, 2022

There's no bug in the C code, you can't split CPU features sensitive stuff into separate Nim files anymore and use localpassc (Weave mratsim/weave#171 (comment) and Constantine: https://github.com/mratsim/constantine/blob/master/constantine/arithmetic/assembly/limbs_asm_mul_x86_adx_bmi2.nim#L30) or nim.cfg anymore (Arraymancer: https://github.com/mratsim/Arraymancer/blob/master/nim.cfg#L83)

@Araq
Copy link
Member Author

Araq commented Jan 10, 2022

There is no logic that "inlines" Nim modules and the existing logic didn't change. I'm not saying "there is no problem here", I'm saying "please give me a reduced test case".

@ringabout
Copy link
Member

ringabout commented Jan 11, 2022

A reduced example

https://github.com/xflywind/reduced

example.nim

import macros

type
  MicroKernel = object
    cpu_simd: CPUFeatureX86

  CPUFeatureX86* = enum
    x86_AVX512

{.localpassC:"-mavx512f -mavx512dq".}

func x86_ukernel*(cpu: CPUFeatureX86, T: typedesc): MicroKernel =
  result.cpu_simd = cpu

{.pragma: x86_type, byCopy, header:"<x86intrin.h>".}
{.pragma: x86, noDecl, header:"<x86intrin.h>".}
type
  m512d {.importc: "__m512d", x86_type.} = object
    raw: array[8, float64]

func mm512_setzero_pd(): m512d {.importc: "_mm512_setzero_pd", x86.}

macro ukernel_generator(simd: static CPUFeatureX86): untyped =
  result = newStmtList()
  block:
    let ukernel_name = newIdentNode("gebb_ukernel_" & $float64 & "_" & $simd)
    result.add quote do:
      proc `ukernel_name`[ukernel: static MicroKernel]() =
        let AB = ukernel_simd_impl(ukernel)

macro ukernel_simd_impl(ukernel: static MicroKernel): untyped =
  result = newStmtList()
  var rAB = nnkBracket.newTree()
  var rABi = nnkBracket.newTree()
  rABi.add genSym(nskVar, "AB" & $0 & "__" & $0)
  rAB.add rABi

  var declBody = newStmtList()
  let ab = rAB[0][0]
  declBody.add quote do:
    var `ab` = mm512_setzero_pd()

  result = quote do:
    `declBody`
    `rAB`

ukernel_generator(x86_AVX512)

{.experimental: "dynamicBindSym".}

macro dispatch_general*(
    ukernel: static MicroKernel,
  ): untyped =
  result = newStmtList()
  let simdTag = $ukernel.cpu_simd
  let ukernel_name = bindSym("gebb_ukernel_" & $float64 & "_" & simdTag)
  result.add quote do:
    `ukernel_name`[ukernel]()

test.nim

import example

const ukernel = x86_AVX512.x86_ukernel(float64)
dispatch_general(ukernel)

nim c test.nim works in 1.4.8 but breaks in devel.

D:/mingw64/lib/gcc/x86_64-w64-mingw32/9.2.0/include/avx512fintrin.h:325:1: error: inlining failed in call to always_inline '_mm512_setzero_pd': target specific option mismatch
  325 | _mm512_setzero_pd (void)
      | ^~~~~~~~~~~~~~~~~
C:\Users\blue\nimcache\test_d\@mtest.nim.c:654:11: note: called from here
  654 |  AB13_1 = _mm512_setzero_pd();

@ringabout
Copy link
Member

see also #19363

@ringabout ringabout mentioned this pull request Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants