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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added inline comments.


================
Comment at: flang/unittests/Lower/OpenMPLoweringTest.cpp:86
   auto insertPt = mlirOpBuilder->saveInsertionPoint();
+  mlir::Value numThreads;
+  mlir::Attribute defaultValue, procBindValue;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87846