Page MenuHomePhabricator

[OpenMP] Add test for custom state machine if have reduction
ClosedPublic

Authored by jdenny on Nov 12 2021, 9:43 PM.

Details

Summary

D113602 broke the custom state machine when a reduction is present, as
revealed by the reproducer this patch adds to the test suite. In that
case, openmp-opts changes the return value to undef in
__kmpc_get_warp_size (which the custom state machine calls as of
D113602). Later optimizations then optimize away the custom state
machine code as if all threads are outside the thread block, so the
target region does not execute. D114802 fixed that but didn't add a
reproducer.

This patch also adds a __OMP_RTL_ATTRS entry for
__kmpc_get_warp_size to OMPKinds.def, which D113602 missed. This
change does not seem to have any impact on the reduction problem.

Diff Detail

Event Timeline

jdenny created this revision.Nov 12 2021, 9:43 PM
jdenny requested review of this revision.Nov 12 2021, 9:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 9:43 PM

Patch looks fine ofc, more worried about the root cause.

I don't see why openmp-opt would be changing warpsize at present, it's an unknown call (before this patch) so should have been ignored. @jdoerfert can you guess where we should look here?

JonChesterfield accepted this revision.Dec 1 2021, 12:57 PM

More robust reduction seems a win, thanks!

This revision is now accepted and ready to land.Dec 1 2021, 12:57 PM
jdoerfert requested changes to this revision.Dec 1 2021, 2:03 PM

I didn't see this patch before, sorry.

D114802 was the right fix.

I am not sure how this fixes anything to be honest.
This foldKernelFnAttribute(A, ""); in particular is beyond me.

This revision now requires changes to proceed.Dec 1 2021, 2:03 PM
jdenny added a comment.Dec 2 2021, 8:39 AM

I didn't see this patch before, sorry.

D114802 was the right fix.

Glad someone fixed it correctly. But that patch doesn't add a regression test. Is there one somewhere? Should I commit the test addition from the current patch?

I am not sure how this fixes anything to be honest.
This foldKernelFnAttribute(A, ""); in particular is beyond me.

I'll remove it.

Is the __OMP_RTL_ATTRS addition in the current patch useful?

jdenny updated this revision to Diff 391385.Dec 2 2021, 10:50 AM
jdenny retitled this revision from [OpenMP] Fix custom state machine if have reduction to [OpenMP] Add test for custom state machine if have reduction.
jdenny edited the summary of this revision. (Show Details)

Removed incorrect fix, but kept other changes, as discussed.

jdoerfert accepted this revision.Dec 8 2021, 4:54 PM

LG, more tests.

This revision is now accepted and ready to land.Dec 8 2021, 4:54 PM