[PATCH] D87846: [OpenMP][flang]Lower NUM_THREADS clause for parallel construct

[Valentin Clement via Phabricator via llvm-commits]

clementval added a subscriber: jeanPerier.
clementval added inline comments.


================
Comment at: flang/unittests/Lower/OpenMPLoweringTest.cpp:86
   auto insertPt = mlirOpBuilder->saveInsertionPoint();
+  mlir::Value numThreads;
+  mlir::Attribute defaultValue, procBindValue;
----------------
SouraVX wrote:
> clementval wrote:
> > What is the plan with those unit tests once the bridge and lit test will be upstream. As I said previously, I don't think those test are testing the actual lowering and the API they use is probably tested in the respective MLIR tests. Are we going to delete them once we have the lit one or do we have to maintain both? 
> > What is the plan with those unit tests once the bridge and lit test will be upstream. As I said previously, I don't think those test are testing the actual lowering and the API they use is probably tested in the respective MLIR tests.
> 
> The plan is to continue this way(if people have nothing against this (including you)). Later at some point when we have the bridge, all we have to do is switch the `builder` and integrate the `API`. 
> Doing all this in one-step(when bridge is up) doesn't sound good.
> 
> > Are we going to delete them once we have the lit one or do we have to maintain both?
> These are just unit-tests, can/should be extended when a new `Op` is introduced or lowered. There won't be much of a maintenance issue that I can think of. 
> But at the same time if community decides/thinks these won't add much value, once regression tests are up and running. Deleting them would be a trivial task, only one file, no associated dependencies and all.
My biggest concern is that these tests are disconnected from the actual code in `flang/lib/Lower/OpenMP.cpp`. I think @jeanPerier raised some concern in the past about those tests as well. If others are happy with them I'll follow the crowd but I have a hard time understanding what those tests are bringing in. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87846