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

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

Thanks for driving this, Diana!

> This is adding ExternalNameConversion to the default FIR codegen pipeline, right before the FIRtoLLVM pass.

Makes sense to me.

> There's one lowering test that needs updating, because it's calling `bbc | tco`. I'm not sure why it needs this end-to-end behaviour, but if it really does I'm guessing it's better to let it run the default passes and update the test accordingly than to explicitly disable this pass.

IMO it would make a lot of sense to use `flang-new -fc1 -emit-llvm` for this test. That would make the intent clearer (i.e. that the goal is to verify the generated LLVM IR). On a di

> Do we still want to add a flag to optionally disable this pass?

Depends on our end goal here. @schweitz mentioned  Windows interoperability. I guess that the mangling will be different on other platforms too? We could add the flag to disable `ExternalNameConversionPass` and then use it in tests so that:

- all tests will auto-magically work on Windows (or any other platform that we care about),
- we won't be adding extra check lines for different mangling (we would require 1 per every supported platform).

We wouldn't use the flag in tests that verify the mangling itself. But otherwise, it would be always set. In general, I think that it would be nice to avoid:

- requiring people to write different `CHECK` lines for different drivers (unless we are verifying that the behavior in some specific scenario is indeed different),
- using regex in `CHECK-LABEL` (it will be just easier for everyone if we don't).

Having said all that, we don't seem to need this flag just now :) But it would be nice to agree what to check in `CHECK-LABEL`.

> Also, do we want to move the pass from `flang/Optimizer/Transforms/Passes.td` to `flang/Optimizer/Codegen/CGPasses.td`, to clarify that it's a codegen pass?

+1


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

https://reviews.llvm.org/D121171