[PATCH] D109635: [WIP][OpenMP] Support construct trait set for Clang

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:746
+        }
+      }
+  }
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > So, SmalVector has a pop_back_val which we could use to verify the properties are the ones we expect. However, we would need to first put them into a vector and if insert is false we would need to reverse the vector.
> > Maybe that's the way to go:
> > 1) Where you call `handleConstructTrait` right now, just put the traits in a vector.
> > 2) Make it `handleConstructTraits` and take a vector of construct traits.
> > 3) For insert == true, just append them all.
> > 4) For insert == falser reverse the vector and do pop_back_val with an assertion that they match the result of vector.pop_back_val();
> > 
> > Does that make sense?
> > 
> > (Apologies for making you rewrite this twice.)
> This is not what I meant and I doubt this works. Let me try to explain with an example:
> 
> `handleDeclareVariantConstructTrait` for a DSKind `parallel_for` will first add a `parallel` then a `for` construct trait.
> When we leave the `parallel for` we should pop them of our construct trait stack again, and preferably verify we do see the ones we expect.
> So let's assume the stack was like this before
> 
> ```
> [ teams ]
> // enter parallel for code generation
> [ teams, parallel, for]
> ...
> // exit parallel for code generation
> ```
> at this point we want to pop `for` and then `parallel` from the end.
> 
> If the last one on the stack was not `for` or the second to last was not `parallel` something went wrong.
> Since `handleDeclareVariantConstructTrait` does give use the traits "in order", thus [parallel, for], we need to do something different to verify this is the suffix of the stack.
> 
> I proposed to accumulate all traits in `handleDeclareVariantConstructTrait` rather than passing them to `handleConstructTrait` one by one.
> So there would be a single call to `handleConstructTrait` at the end with all the traits, here [parallel, for].
> If we are entering the scope we just append them to the stack: [teams] + [parallel, for] = [teams, parallel, for]
> If we are exiting the scope (Insert == false, or name it ScopeEntry instead), we reverse the trait set and compare it against what we push from the stack:
>   stack = [teams, parallel, for] 
>   trait_set = [parallel, for] (passed to `handleConstructTrait` by `handleDeclareVariantConstructTrait) 
>   verification:
>     for (trait in reverse(trait_set)) {
>       stack_last = stack.pop_back_val();
>       assert(trait == stack_last && "Something left a trait on the stack!");
>     }
> 
> Does this make sense?
Thanks for your further explanation and patience, it really helps a lot for me to understand!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109635