Skip to content

fix missing LIBOMPTARGET_DEVICE_ARCHITECTURES in third stage of bootstrap build in LLVM easyblock #3851

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 24, 2025
Merged

fix missing LIBOMPTARGET_DEVICE_ARCHITECTURES in third stage of bootstrap build in LLVM easyblock #3851

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3c3058
LLVM: Fix LIBOMPTARGET_DEVICE_ARCHITECTURES
Thyre Jul 22, 2025
441cc2e
LLVM: Add small doc for `_cmakeopts` and `general_opts`
Thyre Jul 22, 2025
9771da4
Update easybuild/easyblocks/l/llvm.py
Thyre Jul 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion easybuild/easyblocks/l/llvm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,10 @@ def __init__(self, *args, **kwargs):
if self.cfg['use_pic']:
on_opts.append('CMAKE_POSITION_INDEPENDENT_CODE')

# General options being passed to every build stage.
# Here, options that will be required in all build stages should be added.
# Update _cmakeopts in _configure_{general,intermediate,final}_build if
# build option is only relevant for a single build step.
self.general_opts = GENERAL_OPTS.copy()

for opt in on_opts:
Expand Down Expand Up @@ -381,6 +385,9 @@ def __init__(self, *args, **kwargs):
self.log.info("Final projects to build: %s", ', '.join(self.final_projects))
self.log.info("Final runtimes to build: %s", ', '.join(self.final_runtimes))

# CMake options passed to each building stage.
# Will be cleared between stages. If arguments are needed in multiple stages,
# consider adding them to general_opts instead.
self._cmakeopts = {}
Copy link
Contributor

@Flamefire Flamefire Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too late for this PR now, but I thought we could set this to None at this point such that adding dict entries to this too early would fail loudly.

What do you think?

Copy link
Collaborator Author

@Thyre Thyre Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea. We can either add this in a separate PR, or just add it to one of the remaining LLVM ones.

Copy link
Contributor

@Flamefire Flamefire Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #3799 would be a good candidate as IMO that is ready.

@Crivella ?

Copy link
Contributor

@Crivella Crivella Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are already setting it at the edge where we want to access it. If you try to use it before in the init it will be non-existent and fail regardless

Copy link
Contributor

@Flamefire Flamefire Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it must only be used after the _configure_general_build call in configure_step: https://github.com/Thyre/easybuild-easyblocks/blob/9771da4f8969ba373c19b99a6988b3b65113d225/easybuild/easyblocks/l/llvm.py#L833

There cmakeopts should be initialized to a copy of generalopts. Or is there any earlier use intended? Maybe derived easyblocks adding options to that before calling the super-method?

Copy link
Contributor

@Flamefire Flamefire Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I dont understand your remark:

Only if _cmakeopts was not an attribute there which i still think it should.

Getting an error is intended to detect using it too early. And IMO the explicit None shows that it does not exist yet and provides means to add a comment what it is and where it will be set.

Copy link
Contributor

@Crivella Crivella Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What i meant is that i think _cmakeopts should be already usable in the configure step.

I think unless we decide do alter the logic quite a bit this will not really save us from PRs using the wrong attributes/methods if we do not catch it in review...
What i think is more worth the effort is increase the sanity checks to make sure all the feature we want on are on.

Copy link
Contributor

@Flamefire Flamefire Jul 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But should it be usable before _configure_general_build in the configure step?

I'd just make it as hard as possible to use the wrong one if it doesn't impact what we really need and my suggestion is a step towards it while not impacting current usage as far as I can tell.

Better sanity checks would be good either way, yes.

Then maybe the minimal common ground would be renaming the generically named cmakeopts to something more specific like "_stage_cmake_opts"? That would make it easier to review without any real change

Copy link
Contributor

@Crivella Crivella Jul 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But should it be usable before _configure_general_build in the configure step?

I would say so, otherwise we would need a section for updating general_opts based on present packages (what we have right now before _configure_general_build in the configure_step and one to update the single stage after.
I feel that having 2 section there could also lead to confusion and people misplacing stuff (eg modifying general_opts after the call to _configure_general_build

Then maybe the minimal common ground would be renaming the generically named cmakeopts to something more specific like "_stage_cmake_opts"? That would make it easier to review without any real change
Not against renaming variables for clarity, might even make it _current_stage_cmake_opts for clarity

I think the only way to really make sure stuff is not misused is to introduce 4 methods:

  • add_stage1_cmake_opts
  • add_stage2_cmake_opts
  • add_stage3_cmake_opts
  • add_all_stages_cmake_opts

And than we could ensure they are never called after a particular stage has already been ran, and that the internal methods are never touched manually.
This would require quite a lot of refactoring, and high possibility to introduce new errors which i would avoid for now

PS would also require rewriting the logic in how the configopts coming from the EC file now are treated as general_opts + would need to streamline and clarify how stages are treated in the case of bootstrap vs not

Copy link
Contributor

@Flamefire Flamefire Jul 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only way to really make sure stuff is not misused is to introduce 4 methods:

Would it then need to be a property at all? The current member methods that additionally modify it could just a return a dict that is then used to update a local cmakeopts

self._cfgopts = list(filter(None, self.cfg.get('configopts', '').split()))

Expand Down Expand Up @@ -821,7 +828,7 @@ def configure_step(self):
gpu_archs = self.cfg.get_cuda_cc_template_value("cuda_sm_space_sep", required=False).split()
gpu_archs += self.amd_gfx
if gpu_archs:
self._cmakeopts['LIBOMPTARGET_DEVICE_ARCHITECTURES'] = self.list_to_cmake_arg(gpu_archs)
self.general_opts['LIBOMPTARGET_DEVICE_ARCHITECTURES'] = self.list_to_cmake_arg(gpu_archs)

self._configure_general_build()
self.add_cmake_opts()
Expand Down