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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

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

> In D121171#3425946 <https://reviews.llvm.org/D121171#3425946>, @clementval wrote:
>
>> 
>
>
>
>> Most of the passes have a disable option when included in the pipeline. I agree that we might not need it right now but why delay a one line option that will for sure be useful in development and testing.
>
> For testing specific passes, we can already use `fir-opt`. And if we want this pass to be controlled with a flag, then we will need to add it separately in `flang-new` and `tco`. That's going to be more than one line. In general, I think that using `fir-opt` in such scenarios (instead of introducing extra flags) is cleaner and clearer.

We already use fir-opt to test this pass (see here <https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/external-mangling.fir> and here <https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/external-mangling-emboxproc.fir>).

> I'm also wondering - if mangling is going to be platform specific, then perhaps we need options to specify the platform to mangle for instead? This way, we can specify in tests:
>
>   ! RUN: flang-new -fc1 -internal-name-mangling=linux -emit-llvm file.f90
>
> This way, we would be able to keep our tests platform-agnostic (and everything would be future proof). This would also be more flexible than plain "disable"/"enable" toggle. WDTY?

I think that's out of scope for this patch. AFAICT the pass doesn't yet do anything platform specific. When we add support for another kind of mangling, then we can decide how to control it (I would opt for handling it the same way we did for TargetRewrite <https://github.com/llvm/llvm-project/blob/72fe439a4e1187fa47dcdcff32f9c397f21ea25a/flang/test/Fir/target-rewrite-triple.fir#L2>, but there's not much point discussing it right now). Having a flag in tco to disable the pass altogether seems like an orthogonal issue to me, and it would be consistent with the other passes to provide it. I'm uploading a new version of the patch so you can see what I mean (it's only a bit more than one line).


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

https://reviews.llvm.org/D121171