This is an archive of the discontinued LLVM Phabricator instance.

[NFC][OpenMP] Update simd loop collapse support description
ClosedPublic

Authored by domada on Aug 11 2022, 5:47 AM.

Details

Summary

Simdloop collapse clause is supported in the same way as colllapse clause for worksharing loops.

Diff Detail

Event Timeline

domada created this revision.Aug 11 2022, 5:47 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 11 2022, 5:47 AM
domada requested review of this revision.Aug 11 2022, 5:47 AM

nit: add NFC to the title.

flang/test/Lower/OpenMP/simd.f90
123

This is not one good test case for SIMD directive. Can you replace it with the one that can be vectorized?

kiranchandramohan requested changes to this revision.Aug 14 2022, 1:59 PM

As far as possible we should try to have Flang and MLIR OpenMP dialect changes in separate patches.

I would recommend splitting this into two simple patches.

  1. Testing the lowering of OpenMP dialect simdloop collapse to LLVM IR. There is an existing test case, simdloop_multi in the translation tests. You need to strengthen it minimally to check the occurrence of collapse. And adding the collapse information to the OpenMP dialect op description.

The relevant code for this came in the patch https://reviews.llvm.org/D118065.

  1. Test the lowering from source to FIR+OpenMP simdloop with collapse. Add the relevant patch that enabled lowering of the collapse in simdloop. I think the patches that you have provided are not the right ones.
flang/test/Lower/OpenMP/simd.f90
109

All the above checks can either be with the same CHECK line or with the CHECK-SAME directive.

This revision now requires changes to proceed.Aug 14 2022, 1:59 PM
domada updated this revision to Diff 452946.Aug 16 2022, 4:13 AM
domada retitled this revision from [Flang][OpenMP] Add test case for simd collapse clause to [NFC][OpenMP] Update simd loop collapse support description.
domada edited the summary of this revision. (Show Details)

Add only OpenMP specific changes. Update simd loop MLIR description and strengthen translation test.

As far as possible we should try to have Flang and MLIR OpenMP dialect changes in separate patches.

I would recommend splitting this into two simple patches.

  1. Testing the lowering of OpenMP dialect simdloop collapse to LLVM IR. There is an existing test case, simdloop_multi in the translation tests. You need to strengthen it minimally to check the occurrence of collapse. And adding the collapse information to the OpenMP dialect op description.

The relevant code for this came in the patch https://reviews.llvm.org/D118065.

Done. I modified this review so that it contains only OpenMP specific changes.

  1. Test the lowering from source to FIR+OpenMP simdloop with collapse. Add the relevant patch that enabled lowering of the collapse in simdloop. I think the patches that you have provided are not the right ones.

Will be done as the separate patch.

@peixin
Thank you for your review. I will apply your remarks in the separate patch which will be Flang specific.

Thanks @domada, this looks better. I have a comment about the way the test is written.

mlir/test/Target/LLVMIR/openmp-llvm.mlir
719–722 ↗(On Diff #452946)

As you know checking for names is not always the best. Can you instead do something similar to the way the collapse test is written in D105706?

domada updated this revision to Diff 453230.Aug 17 2022, 1:18 AM

Updated test: Do not rely on hardcoded labels. Check if collapsed loop bound is calculated.

domada marked an inline comment as done.Aug 17 2022, 1:19 AM

LGTM. Thanks for making the changes.

This revision is now accepted and ready to land.Aug 17 2022, 1:45 AM
domada marked an inline comment as done.Aug 17 2022, 4:26 AM

Follow up review: https://reviews.llvm.org/D132023 . This review contains Fortran test with simd clause.

flang/test/Lower/OpenMP/simd.f90
109

Done

123