[Openmp-commits] [PATCH] D158544: [OpenMP] Optimized trivial multiple edges from task dependency graph

Wed Aug 23 08:29:27 PDT 2023

rpereira-dev added inline comments.

================
Comment at: openmp/runtime/src/kmp_taskdeps.cpp:333
+                                                     kmp_depnode_t *sink,
+                                                     kmp_depnode_t *source) {
+  if (!source)
----------------
rpereira-dev wrote:
> tianshilei1992 wrote:
> > rpereira-dev wrote:
> > > tianshilei1992 wrote:
> > > > rpereira-dev wrote:
> > > > > jdoerfert wrote:
> > > > > > why is the order here swapped?
> > > > > Order remains the same for callers; the patch only swaps in this prototype as i believe it was mistakenly inverted
> > > > > (used to be `sink`  -> `source`, is now `source` -> `sink` )
> > > > This looks pretty confusing IMO.
> > > Want me to revert this change and only keep the multiple edges removal ? (4 lines 310, 322, 352, 378)
> > > I can make another one just for the variable names which are currently misleading
> > Why the current variable names are misleading? If A depends on B, there is an edge A->B so A is source and B is sink.
> The relation « A depends on B » is usually represented it through the edge B -> A, not A -> B
I believe we will not agree on what is misleading or not today :p
We can probably agree on who the "predecessor" and "successor" refer to, and rename those variables accordingly ?
I can also just revert 'sink' and 'source' names swap from the patch and keep as per before

Let me know

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158544