[PATCH] D121171: [flang] Add ExternalNameConversionPass to flang-new pipeline

Thu Mar 10 05:09:20 PST 2022

rovka added inline comments.

================
Comment at: flang/test/Lower/Intrinsics/abs.f90:2
 ! RUN: bbc -emit-fir %s -o - | FileCheck %s
 ! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s

----------------
awarzynski wrote:
> pmccormick wrote:
> > schweitz wrote:
> > > schweitz wrote:
> > > > awarzynski wrote:
> > > > > schweitz wrote:
> > > > > > Why does this test need to be run with flang?
> > > > > > 
> > > > > > We don't see the LLVM tests being driven by both opt and clang.
> > > > > > We don't see the LLVM tests being driven by both opt and clang.
> > > > > 
> > > > > That would require LLVM depending on Clang. Also, `opt` is for testing (and setting up) each LLVM pass individually. Clang neither needs nor supports this level of granularity. Instead, it operates at higher abstraction level, e.g. different optimisation pipelines. The situation with `flang-new` and `bbc` is very different.
> > > > BTW, something appears to be incorrect here.
> > > > 
> > > > Is `-emit-fir` on `flang` supposed to behave the same way as it always has with the lowering test tool, `bbc`? The //codegen// pass to convert symbols to some target for compatibility is not part of lowering and isn't reflected in the lowering output which is what `bbc -emit-fir` produces.
> > > > 
> > > > So it looks like `flang -emit-fir` and `bbc -emit-fir` have already diverged here. Is that the intention? If so, then `flang` certainly cannot replace `bbc`.
> > > > That would require LLVM depending on Clang. Also, `opt` is for testing (and setting up) each LLVM pass individually. Clang neither needs nor supports this level of granularity. Instead, it operates at higher abstraction level, e.g. different optimisation pipelines. The situation with `flang-new` and `bbc` is very different.
> > > 
> > > Disagree. `bbc` is a test tool that is intended to test lowering. Specifically, the conversion of the PFT, plus other data structures, to FIR. The purpose of the `flang-new` driver is to be a Fortran compiler end-to-end. Those two objectives are absolutely, positively different.
> > > 
> > > There is clearly confusion on this point. That likely stems from the fact that the only way to pickle the PFT (and other data) is to pretty print it out as Fortran. That perfect circularity makes it appear that `bbc` is trying to be something it was not intended to be.
> > > 
> > > Can we stop conflating these two purposes?
> > @schweitz -- thanks for the clarification on `bbc`.   I have to admit I was confused by the differences/similarities.  Its appearance in use cases that don't feel that different from the driver have probably helped add to the confusion (at least mine! ;-).
> > 
> > Should we consider some way to make this distinction more clear for those new to the code base?   I suspect there is a longer history here behind `bbc` but being more specific about its intended usage might help us avoid revisiting this again down the road; especially so for those new to the code base and the supporting toolset. 
> @schweitz , @pmccormick The motivations for developing `bbc` and `flang-new` were different: `bbc` was developed to test lowering, `flang-new` was introduced as the end-to-end compiler driver. However, we reached a point where `bbc` and `flang-new` overlap in functionality. Strongly. Both `bbc` and `flang-new` consume Fortran source and lower it to MLIR. I'm not aware of anything that `bbc` can do that `flang-new` couldn't. The fact that we can replace `bbc` with `flang-new` in tests demonstrates this point. So yes, the motivations for creating `bbc` and `flang-new` were different, but the available functionality is not.
> 
> Anyway, I think that we are diverging here. A lot :) IIRC, this patch is to discuss `ExternalNameConversionPass`. 
> 
> > So it looks like flang -emit-fir and bbc -emit-fir have already diverged here. 
> Not yet and I agree that we should avoid this :) IIRC, this patch (in its current form) simply demonstrates one approach to facilitate the discussion. @rovka  suggested 2 approaches - both would lead to `flang-new` and `bbc` being incompatible. I suggested 3rd approach, which would keep the 2 tools interchangeable. We are yet to decide which approach to take :)
>  I'm not aware of anything that bbc can do that flang-new couldn't.

In fact there seem to be things that `flang-new` can do that `bbc` can't (see [[ https://github.com/flang-compiler/f18-llvm-project/blob/dd7c0e999f4dc8b56578df6ed70762082589df8e/flang/test/Lower/intrinsic-procedures/exit.f90#L2 | this comment]]). But that's probably for a different patch.

> So it looks like flang -emit-fir and bbc -emit-fir have already diverged here. Is that the intention?

No, in light of the above discussion I'm starting to think this wasn't the right place to add the pass. Since you say it's a codegen pass, would it make more sense for it to be run as part of the [[ https://github.com/llvm/llvm-project/blob/697f55e36823dbd91ca94a666d99f3c4ba11cacb/flang/include/flang/Tools/CLOptions.inc#L163 | defaultFIRCodeGenPassPipeline ]]? Or do we want to keep it just in the driver, maybe right before the other [[ https://github.com/llvm/llvm-project/blob/a278250b0f85949d4f98e641786e5eb2b540c6b0/flang/lib/Frontend/FrontendActions.cpp#L427 | MLIRToLLVM ]] passes?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121171