Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support LLVM Flang (flang-new) #131

Merged
merged 13 commits into from
Dec 19, 2024
Merged

Support LLVM Flang (flang-new) #131

merged 13 commits into from
Dec 19, 2024

Conversation

rouson
Copy link
Collaborator

@rouson rouson commented Sep 6, 2024 •
edited
Loading

This PR

  1. Fixes a code conformance issue that flang identified.
  2. Switches a sync all statement to call prif_sync_all()
  3. Updates install.sh to add flags required for assumed-rank support if llvm is detected in $FPM_CC --version.

fpm test fails due to a code conformance problem in Veggies. An issue will be submitted on the Veggies repository.

@bonachea bonachea self-requested a review September 6, 2024 21:21
bonachea
bonachea requested changes Sep 6, 2024
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor request, otherwise LGTM

src/caffeine/collective_subroutines/co_reduce_s.f90 Outdated Show resolved Hide resolved
src/caffeine/collective_subroutines/co_reduce_s.f90 Outdated Show resolved Hide resolved
@bonachea bonachea mentioned this pull request Sep 6, 2024
Closed
bonachea
bonachea approved these changes Sep 6, 2024
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think we are deliberately omitting documentation for now, at least until we get flang-new into CI and fix the current failures on the co_min/max tests.

bonachea
bonachea requested changes Sep 6, 2024
View reviewed changes
test/caf_co_max_test.f90 Outdated Show resolved Hide resolved
@everythingfunctional
Copy link
Member

I think the tests that are failing are due to the line result_ = assert_that(all(scramlet == co_max_scramlet),"all(scramlet == co_max_scramlet)"), which should use expected_scramlet instead. It may be worth adding 1, 2 and 3d arrays of strings for assert_equals to veggies, as showing the actual arrays in the failure message would make this easier to diagnose.

@bonachea
Copy link
Member

@rouson do you want to cherry-pick the commits from PR #142 into this one, or does it make sense to keep them as separate pull requests?

@rouson
Copy link
Collaborator Author

rouson commented Sep 17, 2024

@rouson do you want to cherry-pick the commits from PR #142 into this one, or does it make sense to keep them as separate pull requests?

@bonachea I just tagged you in a comment on PR #142 and merged that one. @ktras is taking over development on this branch.

@ktras you might want to merge main into this branch or rebase this branch off of main, which would likely go through cleanly because the new commit that I just merged into main is orthogonal to the commits on this branch as far a I recall.

@ktras
Copy link
Collaborator

ktras commented Sep 17, 2024

@rouson I think will rebase this branch with main once PR #143 is merged in main. Because we need those changes on this branch as well.

rouson and others added 5 commits December 17, 2024 12:03
@rouson @everythingfunctional
fix(co_reduce): correctly set character ptr length
493c39a 
This fixes a previously non-conformant way of
setting a character pointer length (a standard
constraint prohibits passing an actual argument
with a deferred length to c_f_pointer).
@rouson @everythingfunctional
fix(install.sh): if flang, add assumed-rank flags
2c6e8ff
@rouson @bonachea @everythingfunctional
Apply suggestions from code review
56e15ac 
fix(co_reduce): rm unused variable

Co-authored-by: Dan Bonachea <[email protected]>
@rouson @everythingfunctional
test(collectives): work around flang bugs
9a132d3
@rouson @everythingfunctional
build(fpm): switch to debug builds
72f1a5e
@ktras ktras requested a review from bonachea December 17, 2024 19:32
bonachea
bonachea requested changes Dec 17, 2024
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted what looks like some scale-dependent regressions in the test code.

Otherwise LGTM

test/caf_co_max_test.f90 Outdated Show resolved Hide resolved
test/caf_co_reduce_test.f90 Outdated Show resolved Hide resolved
ktras
ktras reviewed Dec 17, 2024
View reviewed changes
install.sh Show resolved Hide resolved
bonachea
bonachea approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM and appears to be passing gfortran CI now

ktras
ktras approved these changes Dec 19, 2024
View reviewed changes
Copy link
Collaborator

@ktras ktras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Awesome and thanks @everythingfunctional

@everythingfunctional everythingfunctional merged commit 187a188 into main Dec 19, 2024
6 checks passed
@everythingfunctional everythingfunctional deleted the fix-stop branch December 19, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktras ktras ktras approved these changes

@bonachea bonachea bonachea approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rouson @everythingfunctional @bonachea @ktras