Add build option for ILP64 - #184
Conversation
ef09add to
ac6e9fc
Compare
7e1f82a to
b06c721
Compare
ac6e9fc to
b39b64b
Compare
fixes compiler warning about unused function
b06c721 to
cacc4ab
Compare
There was a problem hiding this comment.
Pull request overview
Adds a Meson build-time option to compile mkl-service against MKL’s ILP64 interface, and uses that option to conditionally compile the ILP64 vs LP64 init path in the Linux preload/init extension to avoid unused-function compiler warnings.
Changes:
- Introduces a new Meson boolean option
ilp64inmeson.options. - Wires
-DMKL_ILP64into the Mesonc_argswhen the option is enabled. - Gates
_set_mkl_ilp64()/_set_mkl_lp64()compilation and selection inmkl/_mklinitmodule.c.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mkl/_mklinitmodule.c |
Conditionally compiles and selects ILP64 vs LP64 init helper to avoid unused code paths and warnings. |
meson.options |
Adds a new ilp64 boolean build option. |
meson.build |
Attempts to add -DMKL_ILP64 to compiler args when the option is enabled. |
|
CI failures are unrelated to PR content and are likely some kind of server issue |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Thank you @ndgrigorian
I added few more comments nits, but all of them are minor and can be dropped.
| | Option | Type | Default | Description | | ||
| | ------- | ------- | ------- | ------------------------------------ | |
There was a problem hiding this comment.
seems the table is missaligned:
| | Option | Type | Default | Description | | |
| | ------- | ------- | ------- | ------------------------------------ | | |
| | Option | Type | Default | Description | | |
| | ------- | ------- | ------- | ------------------------------------- | |
| c_args = ['-DNDEBUG'] | ||
|
|
||
| if get_option('ilp64') | ||
| c_args += '-DMKL_ILP64' |
There was a problem hiding this comment.
Would it be helpful to add a comment "correct only for MKL_LINK=sdl; for static/dynamic, also drive the interface library via MKL_INTERFACE"?
There was a problem hiding this comment.
Or probably even to add setting MKL_INTERFACE=ilp64 with SDL, since it makes no effect with SDL, but will work for static/dynamic.
There was a problem hiding this comment.
And adding MKL_INTERFACE=lp64 to off explicitly when ilp64=false
| c_args = ['-DNDEBUG'] | ||
|
|
||
| if get_option('ilp64') | ||
| c_args += '-DMKL_ILP64' |
There was a problem hiding this comment.
Also, it seems meson is going to be more strict:
The SDL defaults to the LP64 interface and Intel OpenMP
('iomp') threading, with switching other interface and threading options
being accessible at runtime. When using the'sdl'module, either leave out
the interface and threading modules or use values that match the SDL defaults -
anything else will raise an error.
and will not planning to automate sdl + ilp64 combination.
If anything, it's worth a note to the meson PR that SDL+ILP64 is a legitimate Intel-supported configuration that their current model can't express
This PR adds a
meson.optionsfile and adds a build option for ILP64.With this option added, the definition of functions for initializing MKL with the ILP64 interface are gated by ifdefs, silencing a compiler warning