Update test to MLIR

Add assert for bounds back in, remove if-statment. This puts the code in
the new loop back to how the code was before the loop.

Replace if with assert, as it was in earlier code (before the loop was added)

In D125302#3512417, @shraiysh wrote:

LGTM with comments addressed. Thanks!

Update for review comments

In D125407#3507337, @mehdi_amini wrote:

Just some quick skimming through, I had a few comments, hope this helps! :)

LGTM

I have left my machine to compile that file over night, to see if it's a genuine hang or just "takes a very long time" - if it's not finished by tomorrow morning, I'm definitely calling it a hang. I will report back tomorrow on the result of that.

We did some more digging and it looks like a Clang 13 issue. On AArch64, both Clang 12 and Clang 14 are fine. I believe that @Leporacanthicus was also able to reproduce this with Clang 13 on X86.

LGTM

LGTM. Just one nit-pick, there's a few new tests that test some pretty specific things. Would be nice to have a line or three about what they actually test - e.g "Tests that destructor is not called until after OpenMP section has completed".

Generally looks good to me. Just one nit-pick and one "I don't really understand how this actually produces the correct result" (may be that I'm just missing something!)

Make simd_modifier a unit-attribute and fix formatting

Fix review comments

As this is now squashed into D111051, this review is no longer required.

Squashed the "fixes" commit and the original commit

BUMP.

Update to match changes in dependent commit (+ rebase)

Rebase to latest llvm/main

LGTM

Looks OK to me, but please don't merge until someone else also approves.

LGTM.

Updating according to review comments

LGTM

Trying to get two commits in one arcanist command - may not work

Ah, this definitely would make "random" values for floating point values - in weird ways that are hard to understand!

In D108622#2962282, @jeanPerier wrote:

In D108622#2962219, @Leporacanthicus wrote:

Looks like it's doing what's needed. Is there a follow-on patch to fix the fact that values isn't being passed to the DATE_AND_TIME subroutine?

Do you mean a patch in lowering ?

If so, yes, once this is up streamed, lowering will be updated.

Looks like it's doing what's needed. Is there a follow-on patch to fix the fact that values isn't being passed to the DATE_AND_TIME subroutine?

(Sorry, I thought I'd approved it yesterday, but clearly didn't click the submit button!)

It would seem that your patch may have broken some builds:
https://lab.llvm.org/buildbot/#/builders/175/builds/2077

In D107575#2929026, @Meinersbur wrote:

There seems to be a problem in how import common not finding common.py in a sibling directory.

vsnprintf is named _vsnprintf by Microsoft. According to their docs, it should also be available as vsnprintf with

#include <stdio.h>
#include <stdarg.h>

but even Google test uses

#if GTEST_OS_WINDOWS
# define vsnprintf _vsnprintf
#endif  // GTEST_OS_WINDOWS

Thanks for the updated patch, this seems to work for my simple use-case.

In D105706#2905915, @Meinersbur wrote:

In D105706#2904731, @Leporacanthicus wrote:

Calling collapseLoops in convertOmpWsLoop sets the AfterIP to the wrong place - this causes incorrect LLVM-IR to be generated - an empty block and one with two unconditional branches in a row, like this:

Where does AfterIP point to and where do you think it should point to after collapseLoops? Should the result of collapseLoops be changed or is it an issue of how convertOmpWsLoop uses CanonicalLoopInfo?

Sorry for the rather lengthy comment. I am not sure if I'm expecting the wrong thing, I'm doing something wrong, or this patch is incorrect, so I'll try to put as much information into this comment as I can.
Any suggestions, hints or similar would be welcome. I fully expect some of my workarounds and fixes to be called out as wrong - and I don't mean my global variable, which is clearly just a workaround for not understanding how to get the correct value out of where it is known.

LGTM

Some minor nit-picks, but nothing wrong - I tested this with some of my fortran code that I've been using for other testing, and it seems to work.

In D103177#2791916, @klausler wrote:

I still don't understand why you have to mess with f18.cpp. What would break if you left it alone?

Removed wrong comment endings.

@Meinersbur Is there anything else I need to do here?

Remove the code that add in modifiers to the scheduling type,
as per review comments.

Rebase and fixed comment style.

Patch doesn't apply because it is relying on my previous patch. Doh!

LGTM

Adding MLIR to LLVM-IR tests. The tests are very basic, just checking that
the correct runtime functions are called.

Updates based on review comments:

• Fix style using PascalCase rather than camelCase.
• Add Doxygen comment for parameter.
• Change comment style for constants.
• Remove useless error output - there was no way to get to that error.
• Use llvm_unreachable instead of assert(0).

Will do some minor fixes tomorrow, thanks for the feedback!

LGTM

LGTM

LGTM.

In D97393#2666954, @Meinersbur wrote:

Could you add the following to the unittest?

Builder.restoreIP(EndIP);
Builder.CreateRetVoid();
OMPBuilder.finalize();
EXPECT_FALSE(verifyModule(*M, &errs()));

I checks whether the IR is internally consistent.

Updates as per review comments:

Updates requested in review:

• Avoid making temporary string
• Avoid British English in comment
• Be consistent and not mixing auto/named type in new tests

Add testing for chunk size in dynamic work sharing loop test.

LGTM

Nice test, thanks for adding that!

In D99198#2645402, @klausler wrote:

In D99198#2645322, @Leporacanthicus wrote:

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

It gets exercised pretty thoroughly by the I/O tests in the FCVS Fortran '77 suite, especially by codes doing direct and sequential I/O, and the flang/unittests/Runtime/external-io.cpp test really shook it out on the read path (and still passes with this change now that it's used on the write path). But it would be nice to be able to make the code work harder by e.g. using an artificially small buffer size. I've done that in the past during development (but obviously not for console output, sorry) but I'm not sure how it could be made part of a unit testing framework.

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

Fixes as per review comments

Fixed various review comments.

I'm going to upload the fixes once they compile, but it's getting late, so probably not until tomorrow morning.

In D97393#2585126, @Leporacanthicus wrote:

Please note that this is not intended as the final commit, but rather a basis for asking some advice on how to move forward.

The main stumbling point, which may be my lack of understanding of what it's supposed to do: the CanonicalLoopInfo assumes that the the cond block has a CmpInst as the first instruction. In the dynamic, the corresponding block [in my understanding] starts with a call instruction to fetch the "next" set of data to process. This causes the assertOK to fail, hence it is commented out on line 1300 in the patch.

Updated files based on selected review commets.
Fixed a few issues with the code-generation.
Added a basic test (similar to static workshare loop.

Please note that this is not intended as the final commit, but rather a basis for asking some advice on how to move forward.