[PATCH] D83635: [OpenMPOpt][WIP] Merge parallel regions

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I'll have to look at the logic later today. Some initial comments below.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1512
+    return PreservedAnalyses::none();
+  else
+    return PreservedAnalyses::all();
----------------
Style: No else after return.

Please commit this separately, LGTM for this change.


================
Comment at: llvm/test/Transforms/OpenMP/parallel_deletion.ll:416
+; CHECK-NEXT:    ret void
+;
 entry:
----------------
Can you run the update script on the test and commit the change as an NFC right away. Weird that the test claims it is generated but no check lines are here.


================
Comment at: llvm/test/Transforms/OpenMP/parallel_region_merging.ll:245
+attributes #0 = { noinline nounwind optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { noinline norecurse nounwind optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind }
----------------
Remove all oft the attributes if possible, at least all the string attributes. It is also weird that we have `optnone` here and actually do something, I guess OpenMP opt doesn't properly call skipSCC/skipFunction? Remove it here (to make it a problem for tomorrow).


================
Comment at: llvm/test/Transforms/OpenMP/parallel_region_merging.ll:13
+;    void merge_all() {
+;    #pragma omp parallel
+;        {
----------------
hfinkel wrote:
> ggeorgakoudis wrote:
> > hfinkel wrote:
> > > Do these need to say 'nowait' in order to actually match the code?
> > No, the code matches the IR generation ('nowait' is available only for worksharing constructs). The merging transformation emits an explicit barrier in place of the implicit barrier after merging. However, I added a TODO because it could make sense to avoid explicit barrier generation, if the 'nowait' clause is present in an enclosing worksharing construct, and the merged parallel regions are independent.
> Oh, right. Indeed. Thanks.
> 
> We should do barrier elimination anyway, so that's certainly a good point that we should make sure that the barrier elimination can apply to these kinds of barriers (or we can not emit them in the first place, although maybe it's easier to do after we have anything together where we can just query the MemorySSA use/def chain or similar to look at underlying dependence information).
FWIW, while my proposal for `parallel nowait` was briefly discussed, I did not spend enough time on it to get it into OpenMP 5.1. The questions how to synchronize with the threads afterwards and what guarantees you have are interesting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83635/new/

https://reviews.llvm.org/D83635