[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added a comment.

@kiranchandramohan @JonChesterfield  I will address the comments just made, if you think this is otherwise fine it would be good if you accept the patch (it's been around for weeks so it's not really people didn't have a chance to look).



================
Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272
+
+  template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); }
+
----------------
kiranchandramohan wrote:
> jdoerfert wrote:
> > jdoerfert wrote:
> > > kiranchandramohan wrote:
> > > > jdoerfert wrote:
> > > > > kiranchandramohan wrote:
> > > > > > jdoerfert wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > Compiler throws up this error.
> > > > > > > > error: explicit specialization in non-namespace scope ‘class clang::ASTRecordReader’
> > > > > > > Oh, my compiler was happy. Let me rebase and see what the pre-merge bots say so I might get some insight into the problem.
> > > > > > Were you able to reproduce the error? I was using gcc 9.2 compiler.
> > > > > I have not seen the error yet. I build with clang. Do you happen to have an idea how to refactor this? I will look into it with a new gcc asap.
> > > > Moving the specialization to the source file seems to fix the issue.
> > > I will do that then. Did you find the time to look over the rest?
> > > Moving the specialization to the source file seems to fix the issue.
> > 
> > Like this?
> You can remove the following declaration from the header file and just define it in the cpp file. 
>  /// Specialization for OMPTraitInfo*.
>   template <> OMPTraitInfo *readUserType();
Right, will do that, thx!


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:6575
 
+template <> void ASTRecordWriter::writeUserType(OMPTraitInfo *TI) {
+  writeUInt32(TI->Sets.size());
----------------
kiranchandramohan wrote:
> Had to also move this up in the file to avoid an instantiation after use error.
I didn't get that but I have it in the header, I'll figure it out.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:417
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  S.pop_back();
+  return S;
----------------
kiranchandramohan wrote:
> If there is no match? Or is it always guaranteed to have a match?
We always have selector sets (the outermost category). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71830