[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:
> 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. 
Thanks for bringing more audience. Let's make a decision here as to whether we want this or not.
If not, I'm okay with that :) too.
If we decide against it, I'll take the responsibility of the cleanup:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87846