This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Support common block in OpenMP private clause
ClosedPublic

Authored by NimishMishra on Jun 7 2022, 6:31 AM.

Details

Summary

This supports the common block in OpenMP private clause by making each
common block member host-associated privatization and adds the test case.

Diff Detail

Event Timeline

peixin created this revision.Jun 7 2022, 6:31 AM
peixin requested review of this revision.Jun 7 2022, 6:31 AM
peixin planned changes to this revision.Oct 1 2022, 7:47 PM

Need rebase

peixin updated this revision to Diff 464611.Oct 3 2022, 12:29 AM
peixin edited the summary of this revision. (Show Details)

Rebase

peixin added a comment.Jul 7 2023, 8:51 AM

@kiranchandramohan Hi Kiran, could you please help commandeer this patch? I cannot continue working on it due to work changes.

@kiranchandramohan @peixin If it is okay, I can rebase and merge this patch

peixin added a comment.Jul 8 2023, 6:03 AM

@kiranchandramohan @peixin If it is okay, I can rebase and merge this patch

Sure. Thanks, Nimish. But I think this needs review before landing. @kiranchandramohan What do you think?

@kiranchandramohan @peixin If it is okay, I can rebase and merge this patch

Sure. Thanks, Nimish. But I think this needs review before landing. @kiranchandramohan What do you think?

Thanks @peixin for your work here. If @NimishMishra can rebase, then I will review it.

Hello @kiranchandramohan and @peixin. I was working towards rebasing this patch. I observed that createHostAssociateVarClone has changed considerably. Basically, present version of createHostAssociateVarClone has none of the following operations:

mlir::Region &curRegion = getFirOpBuilder().getRegion();
mlir::Value oldVal = fir::getBase(getExtendedValue(hsb));
mlir::Value cloneVal = fir::getBase(exv);
for (auto &oper : curRegion.getOps()) {
  for (unsigned int ii = 0; ii < oper.getNumOperands(); ++ii) {
    if (oper.getOperand(ii) == oldVal) {
      oper.setOperand(ii, cloneVal);
    }
  }
}

As such, I was wondering if it would be a better approach to tackle this problem as done here: https://github.com/NimishMishra/f18-llvm-project/commit/10e5acf5ed91375b651951a384cd04b7029b36e8. Basically, instead of working with createHostAssociateVarClone, we work with the argument to this function. Let me know if this sounds cleaner. Or whether there can be some problems in this approach?

Hello @kiranchandramohan and @peixin. I was working towards rebasing this patch. I observed that createHostAssociateVarClone has changed considerably. Basically, present version of createHostAssociateVarClone has none of the following operations:

mlir::Region &curRegion = getFirOpBuilder().getRegion();
mlir::Value oldVal = fir::getBase(getExtendedValue(hsb));
mlir::Value cloneVal = fir::getBase(exv);
for (auto &oper : curRegion.getOps()) {
  for (unsigned int ii = 0; ii < oper.getNumOperands(); ++ii) {
    if (oper.getOperand(ii) == oldVal) {
      oper.setOperand(ii, cloneVal);
    }
  }
}

As such, I was wondering if it would be a better approach to tackle this problem as done here: https://github.com/NimishMishra/f18-llvm-project/commit/10e5acf5ed91375b651951a384cd04b7029b36e8. Basically, instead of working with createHostAssociateVarClone, we work with the argument to this function. Let me know if this sounds cleaner. Or whether there can be some problems in this approach?

The code you are pointing to above was a temporary HACK to support the privatisation of the worksharing loop operands. Its removal would not otherwise alter the functionality of createHostAssociateVarClone. If this patch works without big changes, I would prefer that. You can always submit an improvement after that.

NimishMishra commandeered this revision.Jul 18 2023, 7:42 AM
NimishMishra updated this revision to Diff 541529.
NimishMishra edited reviewers, added: peixin; removed: NimishMishra.

Rebasing.

NimishMishra added inline comments.Jul 18 2023, 7:45 AM
flang/lib/Lower/OpenMP.cpp
206 ↗(On Diff #541529)

@kiranchandramohan Do you have any suggestion on how to figure out whether a common block is private or firstprivate at this place in the code?

I looked at the flags for the symbols as well, but unfortunately, we don't have any way to do this as far as I can see. I have marked two test cases as XFAIL to let the build pass, but we need a better solution at this.

@kiranchandramohan Can you take a look at this patch when you are free?

LG.

flang/lib/Lower/Bridge.cpp
456–457

Nit: Add braces here.

This revision is now accepted and ready to land.Jul 27 2023, 4:54 AM

One of the gfortran-testsuite tests started failing after this. I have disabled the test for now.

https://lab.llvm.org/buildbot/#/builders/198/builds/4206

https://github.com/llvm/llvm-test-suite/commit/867cb3ed7ee45376799d4d4af418aee31b6596da

I went through the test at https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/gomp/appendix-a/a.23.4.f90.

Seems like the same variable can't be both in common block as well as in a separate private(.). Is that so?

In any case, should I create an issue and fix this? It seems to be a semantics check with common blocks, and hence perhaps aligns with OpenMP 1.1.