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

[Diana Picus via Phabricator via flang-commits]

rovka added a comment.

In D121171#3429309 <https://reviews.llvm.org/D121171#3429309>, @awarzynski wrote:

> Note that without this change, "flang/test/Lower/common-block.f90" can be run with both `bbc`/`tco` and `flang-new`. This change removes this possibility. We pledged to retire bbc/tco in favour of `flang-new`/`fir-opt` and this change is a step in the opposite direction (at least in it's current format) . I think that we should maintain the interoperability of tests using `bbc`/`tco` and `flang-new`.

I'm very much in favor of keeping the interoperability (regardless of whether or not we're going to retire any of the tools). That's why I was asking about how to pass the same flag to flang-new.

>> @awarzynski does flang-new have something that can forward options to MLIR? (I'm thinking of something like -mllvm)
>
> I've recently added `-mllvm`, but I've not had the chance to look at `-mmlir`. To me, it would make sense to add it first and to make sure that "basic-program.fir" works with both `tco` and `flang-new`.
>
> **Edit:** I suspect that with `-mmlir`, you might be able to use the options from CLOptions.inc in `flang-new`. I haven't tried it yet myself.

I think adding -mmlir is the best way to handle this, but that should clearly be a separate patch.

In the meantime, we could add a temporary bandaid flag to flang-new for specifically disabling this pass, but I'm not sure if it's worth the effort, since this isn't the only tco flag that flang-new doesn't support (we already have tests in tree using e.g. --ignore-missing-type-desc or --fir-to-llvm-ir).

To be honest, I'm probably more interested in why these isolated tests need to run the whole pipeline in the first place. I'm not against end-to-end testing where it makes sense, but these 2 tests look like a pretty random subset and could use some comments about what exactly they're trying to achieve.



================
Comment at: flang/test/Lower/common-block.f90:1
-! RUN: bbc %s -o - | tco | FileCheck %s
+! RUN: bbc %s -o - | tco -disable-external-name-interop | FileCheck %s
 
----------------
schweitz wrote:
> This pass will have to be disabled in `tco` by default for tests to pass.
Why? I thought the main purpose of a flag to disable the pass was to facilitate testing.
I'm working from the premise that we want to keep flang-new and tco compatible, which means that if we enable the pass by default in flang-new, we should enable it by default in tco as well.


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

https://reviews.llvm.org/D121171