[PATCH] D69984: [OpenMP][Opt] Annotate known runtime functions and deduplicate more

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added a subscriber: wsmoses.
jdoerfert added a comment.

In D69984#1738384 <https://reviews.llvm.org/D69984#1738384>, @JonChesterfield wrote:

> Some comments on the verbosity of the patch. Definitely like the effect - deduplicating calls where legal is a great result.


Agreed, on both.

> On second thoughts - should these attributes be in the openmp header, instead of inserted by the compiler?

Yes, that would be great. We are actually working on that, see our HTO optimization work @wsmoses presented at LLVM-Dev'19.



================
Comment at: llvm/include/llvm/IR/OpenMPKinds.def:221
                 AttributeSet(), {})
+__OMP_RTL_ATTRS(omp_get_num_threads,
+                AttributeSet(EnumAttr(ReadOnly), EnumAttr(NoUnwind),
----------------
JonChesterfield wrote:
> The repeated expression
> ```AttributeSet(EnumAttr(ReadOnly), EnumAttr(NoUnwind),
>                              EnumAttr(NoFree), EnumAttr(NoSync))```
> is probably worth factoring out into a named variable.
> 
> Starting to approach the point where a tablegen style generator is worthwhile, but I don't think it's there yet
> 
I thought about it and I might just use another macro for it. I'll update once I figured it out.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:106
           *F, RFIs[OMPRTL___kmpc_global_thread_num], GTIdArg);
       Changed |= deduplicateRuntimeCalls(*F, RFIs[OMPRTL_omp_get_thread_num]);
     }
----------------
JonChesterfield wrote:
> ```for (auto op : {OMPRTL_omp_get_thread_num, OMPRTL_omp_get_num_threads,..}) {
> Changed |= deduplicateRuntimeCalls(*F, RFIs[op]);
> }``` ?
Looks better, will do.


================
Comment at: llvm/test/Transforms/OpenMP/add_attributes.ll:1
+; RUN: opt < %s -S -openmpopt        | FileCheck %s
+; RUN: opt < %s -S -passes=openmpopt | FileCheck %s
----------------
JonChesterfield wrote:
> Is there a standard in-tree way of deduplicating stuff like this? It could be a hash of function name to expected attribute, with a generator that writes the IR.
> 
> My last backend generated many thousands of lines of IR from much shorter python scripts. Maybe the in tree equivalent is tablegen?
Can you rephrase this comment, I'm not sure I get it.

(btw. this is for attributes not deduplication)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69984