In D129584#3712537, @tarunprabhu wrote:

In D129584#3673356, @PeteSteinfeld wrote:

@tarunprabhu , this implementation is failing NAG tests. Do you have access to them?

@PeteSteinfeld, no I do not. If you can file a bug report, I can investigate the failure.

In D129316#3712536, @tarunprabhu wrote:

In D129316#3673354, @PeteSteinfeld wrote:

@tarunprabhu , this implementation is causing failures in the NAG tests. Do you have access to them?

@PeteSteinfeld, no I do not. If you can file a bug report, I can investigate the failure.

All builds and tests correctly and looks good.

@tarunprabhu , this is causing NAG tests to fail. Do you have access to them?

@tarunprabhu , this implementation is failing NAG tests. Do you have access to them?

@tarunprabhu , this implementation is causing failures in the NAG tests. Do you have access to them?

All builds and tests correctly and looks good.

LGTM!

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

I notice that other dump options have names that include "debug", for example, "-fdebug-dump-pre-fir". I have no strong preference which way to go, other than we should be consistent. You might want to talk to Jean before submitting these changes.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

Thanks for doing this. Please be sure to correct the typo on line 79 before updating.

Thanks for figuring this out.

These changes look good. But it looks like there are another two instances of this problem in .../flang/lib/Optimizer/CodeGen/TargetRewrite.cpp. Did you take a look at that file?

All builds and tests correctly and looks good. But I don't understand this code very well. Someone with relevant expertise should approve.

Responding to Peter's commend.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

In my previous comments, I mean to request changes!

I get a build error for this change. Here's what I see in the log file:

[5929/6618] Building CXX object tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o
FAILED: tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o 
/home/sw/thirdparty/gcc/gcc-9.3.0/linux86-64/bin/g++ -DFLANG_LITTLE_ENDIAN=1 -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/local/home/psteinfeld/main/temp/build/tools/flang/lib/Optimizer/Dialect -I/local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect -I/local/home/psteinfeld/main/temp/flang/include -I/local/home/psteinfeld/main/temp/build/tools/flang/include -I/local/home/psteinfeld/main/temp/build/include -I/local/home/psteinfeld/main/temp/llvm/include -isystem /local/home/psteinfeld/main/temp/llvm/../mlir/include -isystem /local/home/psteinfeld/main/temp/build/tools/mlir/include -isystem /local/home/psteinfeld/main/temp/build/tools/clang/include -isystem /local/home/psteinfeld/main/temp/llvm/../clang/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -Wno-deprecated-copy -fno-strict-aliasing -fno-semantic-interposition -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o -MF tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o.d -o tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o -c /local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp
/local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp: In static member function ‘static mlir::ParseResult fir::CoordinateOp::parse(mlir::OpAsmParser&, mlir::OperationState&)’:
/local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp:952:58: error: ignoring returned value of type ‘mlir::ParseResult’, declared with attribute nodiscard [-Werror=unused-result]
  952 |   parser.addTypesToList(funcTy.getResults(), result.types);
      |                                                          ^
In file included from /local/home/psteinfeld/main/temp/mlir/include/mlir/Dialect/Arithmetic/IR/Arithmetic.h:13,
                 from /local/home/psteinfeld/main/temp/flang/include/flang/Optimizer/Dialect/FIROps.h:13,
                 from /local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp:13:
/local/home/psteinfeld/main/temp/mlir/include/mlir/IR/OpImplementation.h:1002:15: note: in call to ‘mlir::ParseResult mlir::AsmParser::addTypesToList(llvm::ArrayRef<mlir::Type>, llvm::SmallVectorImpl<mlir::Type>&)’, declared here
 1002 |   ParseResult addTypesToList(ArrayRef<Type> types,
      |               ^~~~~~~~~~~~~~
In file included from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/StorageUniquerSupport.h:17,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/AttributeSupport.h:17,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/Attributes.h:12,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h:13,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/BuiltinAttributes.h:12,
                 from /local/home/psteinfeld/main/temp/flang/include/flang/Optimizer/Dialect/FIRType.h:16,
                 from /local/home/psteinfeld/main/temp/flang/include/flang/Optimizer/Dialect/FIROps.h:12,
                 from /local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp:13:
/local/home/psteinfeld/main/temp/mlir/include/mlir/Support/LogicalResult.h:112:22: note: ‘mlir::ParseResult’ declared here
  112 | class LLVM_NODISCARD ParseResult : public LogicalResult {
      |                      ^~~~~~~~~~~
cc1plus: all warnings being treated as errors
[5930/6618] Building CXX object tools/mlir/lib/Dialect/LLVMIR/CMakeFiles/obj.MLIRLLVMDialect.dir/IR/LLVMDialect.cpp.o

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

Thanks, Valentin!

All builds and tests correctly and looks good.

All builds and tests correctly, but it would be better to have calls to TODO() rather than fatal unsupported messages.

All builds and tests correctly, but it would be better to use calls to TODO() to emit not yet implemented messages.

All builds and tests correctly and looks good.

Aside from a couple of nits on the comments, all builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good to me. But you should get someone more familiar with lowering code to approve.

Looks good to me!

Looks good!

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and tests correctly and looks good.

All builds and test correctly and looks good.

Sorry for the delay. I didn't notice this the first time around.

All builds and tests without error and looks good.

Responding to Peter's latest suggestion.

Thanks @klausler. I'm tried to follow your suggestion. Please take another look.

Responding to Peter's comments. I restricted all changes to extrema.cpp where
I modified TypedPartialMaxOrMinLoc to test for the scalar .FALSE. value of MASK
and, in the case, create the result and set it to all zeroes.

All builds and tests correctly and looks good.

Responding to Jean's comments. Rather than using the presence of a mask in
the ExtremumLocAccumulator object to determine if the MASK argument is a
scalar .FALSE. value, I added an argument to the GetResult function.