Skip to content

Make reference ukernels MR/NR sizes configurable#547

Closed
hominhquan wants to merge 1 commit intoflame:masterfrom
hominhquan:ref_microsizes
Closed

Make reference ukernels MR/NR sizes configurable#547
hominhquan wants to merge 1 commit intoflame:masterfrom
hominhquan:ref_microsizes

Conversation

@hominhquan
Copy link
Copy Markdown
Contributor

  • Those reference micro-sizes are hardcoded for some performance purposes
    (#pragma omp simd). They may work most of the time, except when user begins
    tuning the MR/NR sizes.
  • This commit exposes them as configurable by user in bli_family_.h,
    makes generation of reference ukernels more robust/consistent. Some explanation
    is added to put future developers on guard (should be added in wiki as well).

- Those reference micro-sizes are hardcoded for some performance purposes
(#pragma omp simd). They may work most of the time, except when user begins
tuning the MR/NR sizes.
- This commit exposes them as configurable by user in bli_family_<arch>.h,
makes generation of reference ukernels more robust/consistent. Some explanation
is added to put future developers on guard (should be added in wiki as well).
@hominhquan
Copy link
Copy Markdown
Contributor Author

Update with added doc in docs/ConfigurationHowTo.md (113a1a9)

@devinamatthews
Copy link
Copy Markdown
Member

@hominhquan I'm planning to address this issue within a broader update of the microkernel layer over the next few months. Is having a fix for this issue being merged into master a priority for you?

@hominhquan
Copy link
Copy Markdown
Contributor Author

@devinamatthews Yes this fix is useful to me. It may also prevent other people from falling to the issue, at least in the next few months. So yes I would like to see it merged into master.

@devinamatthews
Copy link
Copy Markdown
Member

@fgvanzee what do you think? The other fix of course is to disable the vectorized reference kernel.

@fgvanzee
Copy link
Copy Markdown
Member

@fgvanzee what do you think? The other fix of course is to disable the vectorized reference kernel.

Is disabling the vectorized reference gemm ukernel, as you propose, really just a matter of changing #if 1 to #if 0 on line 37 of ref_kernels/3/bli_gemm_ref.c? If so, and if that fix works for @hominhquan, then I'm somewhat partial to that solution. But if @hominhquan needs the individual MR/NR macro constants for other purposes beyond disabling the vectorization, then I suppose that fix would not work.

@devinamatthews
Copy link
Copy Markdown
Member

@fgvanzee I believe it is that easy.

@hominhquan
Copy link
Copy Markdown
Contributor Author

@devinamatthews or @fgvanzee Can you tell if this has been addressed in any ongoing development and should I drop this PR ?

@fgvanzee
Copy link
Copy Markdown
Member

@devinamatthews might be able to comment further, but yes, this is part of ongoing development. We've spoken about Devin's idea as it pertains to this topic and I think it will work for your purposes, too, @hominhquan.

@devinamatthews
Copy link
Copy Markdown
Member

@hominhquan after some pending PRs I'm planning to work on overhauling the way kernels are handled. As part of that, I plan to reintroduce the MR/NR macros so that reference kernels can use them to do compile-time optimizations safely.

@hominhquan
Copy link
Copy Markdown
Contributor Author

Ok, so I drop this PR and wait for your coming update.

@hominhquan hominhquan closed this Dec 1, 2021
devinamatthews added a commit that referenced this pull request Feb 6, 2022
The gemm reference kernel now uses the configuration-dependent BLIS_MR_x/BLIS_NR_x macros to control unrolling, rather than fixed values. This fixes #259 and replaces PR #547.
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.

3 participants