This is an archive of the discontinued LLVM Phabricator instance.

[OpenMPOpt] initial tests for ICV tracking. Only nthreads is used.
ClosedPublic

Authored by sstefan1 on Jun 3 2020, 12:54 PM.

Details

Summary

Couple of tests to showcase what will be done and what to expect with ICV tracking.

Diff Detail

Event Timeline

sstefan1 created this revision.Jun 3 2020, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 12:54 PM

Thanks! One comment for the FIXME.

llvm/test/Transforms/OpenMP/icv_tracking.ll
62

I guess %5 should be replaced with min(%3, 10), right? We might want to use SCEV to represent the values and combine them.

Nice. Great to have tests committed before the optimisation as it means the diff attached to that optimisation makes it really obvious what the transform did.

It might be worth removing some more of the noise from the IR, e.g. the 'noalias nocapture' stuff on the arguments is unlikely to be relevant.

sstefan1 marked an inline comment as done.Jun 4 2020, 3:49 AM

It might be worth removing some more of the noise from the IR, e.g. the 'noalias nocapture' stuff on the arguments is unlikely to be relevant.

Agreed. I already cleaned some stuff up. Will do more.

llvm/test/Transforms/OpenMP/icv_tracking.ll
62

Not sure I understand why min(%3, 10)? Without any transformations %5 is 10. Maybe I'm missing something?

Will look into using SCEV.

jdoerfert added inline comments.Jun 4 2020, 7:18 AM
llvm/test/Transforms/OpenMP/icv_tracking.ll
62

In reality, it is a value smaller or equal to 10, I think.

Let's say we have a quad core and the user sets the num_threads to 10, there are two reasonable outcomes. The runtime will cap it at 4, given that any more threads will most likely not help, or it will go with 10. Afaik, OpenMP doesn't guarantee 10 but not more than 10 if you set a limit. We might want to provide user hooks that simplify the logic, e.g., allow us to take 10, but otherwise min(10, %3) seems reasonable to me. WDYT?

sstefan1 marked an inline comment as done.Jun 4 2020, 8:35 AM
sstefan1 added inline comments.
llvm/test/Transforms/OpenMP/icv_tracking.ll
62

Oh, ok. Makes sense now. 10 was totally random. I'll update the comment to have min(10, %3).

sstefan1 updated this revision to Diff 268490.Jun 4 2020, 8:37 AM

changed fixme, IR cleanup

jdoerfert accepted this revision.Jun 15 2020, 2:12 PM

LGTM

llvm/test/Transforms/OpenMP/icv_tracking.ll
3

Run it with old and new pass manager

This revision is now accepted and ready to land.Jun 15 2020, 2:12 PM
This revision was automatically updated to reflect the committed changes.