[Flang] -funderscoring bug fix

[madanial]

Summary

There was a bug with the -funderscoring / -fno-underscoring options from (https://reviews.llvm.org/D140795) that prevented the driver option from controlling the underscoring behaviour and instead the behaviour could only be controlled by the pass option instead of the driver option. The driver test case did not catch the bug and also needed to be updated.

[awarzynski]

Thanks for the fix!

flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp

146

161

What's the difference between appendUnderscore and appendUnderscores? Perhaps you could introduce more descriptive names?

flang/test/Driver/underscoring.f90

12–13

You could add this (and similar checks) to be stricter.

19–20

IIUC, this form does not appear in any of the cases? Then I would not include it - to reduce noise.

[madanial]

Thanks for the review, addressing review comments.

[awarzynski]

LGTM, thanks!

[DavidSpickett]

FYI I just got this test failing on an unrelated change: https://lab.llvm.org/buildbot/#/builders/21/builds/78228
An earlier example that had none of my changes: https://lab.llvm.org/buildbot/#/builders/21/builds/78223

Not sure that it's this change in particular, but it's the most recent thing to point at :)

I looked briefly at Linaro's flang bots and don't see this happening. Would be worth scanning through the driver option for any uninitialised variables, maybe building with UBSAN (I don't know if there is a bot that does that for flang).

[DavidSpickett]

flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp

161

This changes the member variable, is that intentional? I guess that it doesn't make any difference unless we re-use the pass object, but it does look a bit strange.

usePassOpt looks to always be true, but I must be missing something there.

[madanial]

flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp

161

Thanks for looking into this. The ExternalNameConversion pass is controlled by 2 options a pass option for running with tco and fir-opt and a driver option for flang-new. I think the intermittent failure in the test is caused by the uninitialization of usePassOpt, working on confirming that at the moment.

flang/include/flang/Optimizer/Transforms/Passes.td

 
 def ExternalNameConversion : Pass<"external-name-interop", "mlir::ModuleOp"> {
   let summary = "Convert name for external interoperability";
   let description = [{
     Demangle FIR internal name and mangle them for external interoperability.
   }];
   let constructor = "::fir::createExternalNameConversionPass()";
   let options = [
-    Option<"appendUnderscore", "append-underscore",
+    Option<"appendUnderscoreOpt", "append-underscore",
            "bool", /*default=*/"true",
            "Append trailing underscore to external names.">
   ];
 }
 
 def MemRefDataFlowOpt : Pass<"fir-memref-dataflow-opt", "::mlir::func::FuncOp"> {
   let summary =
     "Perform store/load forwarding and potentially removing dead stores.";

flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp

 };
 
 class ExternalNameConversionPass
     : public fir::impl::ExternalNameConversionBase<ExternalNameConversionPass> {
 public:
   ExternalNameConversionPass(bool appendUnderscoring)
       : appendUnderscores(appendUnderscoring) {}
 
-  ExternalNameConversionPass() { appendUnderscores = appendUnderscore; }
+  ExternalNameConversionPass() { usePassOpt = true; }
 
   mlir::ModuleOp getModule() { return getOperation(); }
   void runOnOperation() override;
 
 private:
   bool appendUnderscores;
+  bool usePassOpt;
 };
 } // namespace
 
 void ExternalNameConversionPass::runOnOperation() {
   auto op = getOperation();
   auto *context = &getContext();
 
+  appendUnderscores = (usePassOpt) ? appendUnderscoreOpt : appendUnderscores;
+
   mlir::RewritePatternSet patterns(context);
   patterns.insert<MangleNameOnFuncOp, MangleNameForCommonBlock,
-                  MangleNameOnAddrOfOp>(context, appendUnderscore);
+                  MangleNameOnAddrOfOp>(context, appendUnderscores);
 
   ConversionTarget target(*context);
   target.addLegalDialect<fir::FIROpsDialect, LLVM::LLVMDialect,
                          acc::OpenACCDialect, omp::OpenMPDialect>();
 
   target.addDynamicallyLegalOp<mlir::func::FuncOp>([](mlir::func::FuncOp op) {
     return !fir::NameUniquer::needExternalNameMangling(op.getSymName());
   });

flang/test/Driver/underscoring.f90

 ! Test the -funderscoring flag
 
 ! RUN: %flang_fc1 -S %s -o - 2>&1 | FileCheck %s --check-prefix=UNDERSCORING
 ! RUN: %flang_fc1 -S -fno-underscoring %s -o - 2>&1 | FileCheck %s --check-prefix=NO-UNDERSCORING
 
 subroutine test()
   common /comblk/ a, b
   external :: ext_sub
   call ext_sub()
 end
 
 ! UNDERSCORING: test_
+! UNDERSCORING-NOT: {{test:$}}
 ! UNDERSCORING: ext_sub_
+! UNDERSCORING-NOT: {{ext_sub[^_]*$}}
 ! UNDERSCORING: comblk_
+! UNDERSCORING-NOT: comblk,
 
 ! NO-UNDERSCORING-NOT: test_
-! NO-UNDERSCORING-NOT: _QPtest
-! NO-UNDERSCORING: test
+! NO-UNDERSCORING: test:
 ! NO-UNDERSCORING-NOT: ext_sub_
-! NO-UNDERSCORING-NOT: _QPext_sub
-! NO-UNDERSCORING: ext_sub
+! NO-UNDERSCORING: {{ext_sub[^_]*$}}
 ! NO-UNDERSCORING-NOT: comblk_
-! NO-UNDERSCORING-NOT: _QBcomblk
-! NO-UNDERSCORING: comblk
+! NO-UNDERSCORING: comblk,