@Munesanz once you add that test case, I will land it.

Ping.

In D150962#4356578, @tlwilmar wrote:

I think OMP_ prefix is reserved for environment variables defined by the spec, no? Or is this being added to the spec?

Ping @jdoerfert @tianshilei1992

Other than the comment this looks good to me, cleaner than before. Thanks Kevin.

In D146642#4229078, @tianshilei1992 wrote:

In D146642#4229069, @josemonsalve2 wrote:

In D146642#4229067, @tianshilei1992 wrote:

In D146642#4229048, @josemonsalve2 wrote:

In D146642#4228996, @tianshilei1992 wrote:

I think it's better to guard the entire related code with macro.

What do you mean by this Shilei? To disable this feature?

Like a opt-in feature.

Why do you think so? What's the harm of leaving it enable as this is just an API function? Are you saying this to save space in the structs?

It's not just an API function. It contains many runtime checks which can potentially compromise the performance for users that don't need the feature. I'd prefer to take it similar to OMPT.

In D146642#4229067, @tianshilei1992 wrote:

In D146642#4229048, @josemonsalve2 wrote:

In D146642#4228996, @tianshilei1992 wrote:

I think it's better to guard the entire related code with macro.

What do you mean by this Shilei? To disable this feature?

Like a opt-in feature.

In D146642#4228996, @tianshilei1992 wrote:

I think it's better to guard the entire related code with macro.

Just a suggestion, I think this works, although it may be best to use OpenMPIRBuilder.

Adding some initial comments

Hi Ravi,

Adding another limitation to this approach. In the following code:

Will this solve the problem or mitigate the error?

Thanks for the clarification Shilei

I’m confused that Shilei said in the issue in GitHub that this is supported already. But front end is just throwing an error message.

Minor things

In D133949#4006520, @jdoerfert wrote:

I could not find source declarations for:

kmpc_get_hardware_num_blocks() // kmpc_get_hardware_num_threads_in_block looks similar and is returning uint

We somehow dropped this one and stopped using it. It was introduced in https://reviews.llvm.org/D106390, @josemonsalve2 can you take a look to re-introduce it (separate review).

Thanks for implementing this. I have added a comment inlined.

This is a good idea. Thanks Joseph.

That one is a good point. Let us revise that. Since the effect we actually want to have is the creation of the task graph sequentially.

I agree on 2. Any recommendations? We can move some of the logic there.

CUDA streams are FIFO queues. But it’s true this will not work if the device queue is not FIFO. In the case of CUDA this works for the case you described without being a read after write dependency.

It is only valid under the assumption of non unified shared memory. State across host and device is only visible during data movements. So it is up until then when changes in the host or device data is reflected. Assuming there are no external runtimes, it is possible to synchronize only on data movements, conserving the data dependencies between host and device.

Rebasing to master

I'm not sure what to do about the buildbot errors I am getting. I'm accessing them but there's no info that tells me what's going on.

Fixing var name to coding standard

Changing omp_get_num_devices() for omp_get_initial_device()

Ah! I missed that. Thanks

Fixing the warning issue on the switch statement.

Changes are fine. I am not familiar with the progress in C++20 and 23, but I trust your judgement here.

Ah! Sorry about this. Quick fix. Working on it.

Running clang-format

Thanks Jon for all the comments.

Cleaning up code to remove unused enums. Removed the commented code with cache_iteration, and all related funcitons.

I missed those. I will check well with all of them.

I have added the HSA_ISA_INFO_NAME, made the function static, and removed
the unused elements in the enums.

@saiislam I will look into that.

lgtm

In D110047#3008555, @josemonsalve2 wrote:

Thanks for adding this Michael.

I saw you are adding the tests that are expected to pass in the cmake but made changes to allow expected fails in LIT. Wouldn’t it make sense to have expected failures in cmake as well. It seems more intuitive to me to mention what is not expected to work. If new tests are added and they pass then that’s fine. It’s more those that are added that do not pass that should trigger a warning.

Other than that it looks good.

Thanks for adding this Michael.

Sorry for the delay. Working on this

Removing branch dependency

Rebasing to main this time for real

Resync again

Rebase to main

Rebase to main

Sync to main

Fixing tests

Fixing @tianshilei1992 comments.

Changing name and adding cstdio instead of iostream. Adding license headers. Other minor changes

Updating minor comments. Major re-design of a less verbose solution will be added later

This has been completed by @jhuber6 in D97680

Rebasing to main

Rebasing to main

Fixing final comments from @jdoerfert

I thought about the -omp- in the name too. But I remembered that the ACC folks wanted to use the same runtime. I like both llvm-omp-device-info and llvm-omp-deviceinfo. Or we could drop the llvm- as they do in mlir- tools

My test is still failing, but it fails on an assertion on the changeToSPMD method:

In D106751#2902593, @JonChesterfield wrote:

Not obvious to me that the functionality has much to do with the plugin. Could do a standalone tool instead?

I think there's a tool called nvidia-smi that does something similar. There's definitely one called rocminfo that does. The latter prints 'human readable' output, which gets in the way of scripting with it.

Format

• Created a single function receiving a string instead of one per attribute: foldKernelFnAttribute
• Removed pesimisticFixpoint if no kernel is found.
• Adding Check for the ReachingKernelEntries valid state
• Removed unnecesary comments