[fir] Add data flow optimization pass

[clementval]

Summary

Add pass to perform store/load forwarding and potentially removing dead
stores.

This patch is part of the upstreaming effort from fir-dev branch.

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

49

Not sure why it is a reference, and auto does not help readability so why not spell out the type? (here and everywhere, in all the upstreaming patches please).

53

85

It is a bit misleading that the variable name is loadOps but contains instances of ReadOp (while there is also a LoadOp class...)

107

flang/test/Fir/memref-data-flow.fir

390

This looks like purely generated checks, can you minimize these?

[jdoerfert]

Drive by: Just glancing at this I am very doubtful this is correct or well tested:

• All non-load/store users are ignored, which is already a bad thing (I just so the FIXME which is easily lost and not mentioned in the rather sparse commit message).
• The reasoning about "nearest stores" ignores all non dominating stores, which should also not be correct.
• The reasoning about "unneeded stores" ignores non-dominated loads, which should also not be correct.
• Given that the test have no comments I'm not sure if they are positive or negative tests. I would hope both are present and the situations described above are in there.

[clementval]

Address @mehdi's comments

[clementval]

In D111288#3050087, @jdoerfert wrote:

Drive by: Just glancing at this I am very doubtful this is correct or well tested:

• All non-load/store users are ignored, which is already a bad thing (I just so the FIXME which is easily lost and not mentioned in the rather sparse commit message).
• The reasoning about "nearest stores" ignores all non dominating stores, which should also not be correct.
• The reasoning about "unneeded stores" ignores non-dominated loads, which should also not be correct.
• Given that the test have no comments I'm not sure if they are positive or negative tests. I would hope both are present and the situations described above are in there.

As for probably 80% of the fir-dev upstreaming patch, I'm not the primary author of this code so maybe @schweitz can comment on that.

[clementval]

Rebase

[schweitz]

In D111288#3050436, @clementval wrote:

In D111288#3050087, @jdoerfert wrote:

Drive by: Just glancing at this I am very doubtful this is correct or well tested:

• All non-load/store users are ignored, which is already a bad thing (I just so the FIXME which is easily lost and not mentioned in the rather sparse commit message).
• The reasoning about "nearest stores" ignores all non dominating stores, which should also not be correct.
• The reasoning about "unneeded stores" ignores non-dominated loads, which should also not be correct.
• Given that the test have no comments I'm not sure if they are positive or negative tests. I would hope both are present and the situations described above are in there.

As for probably 80% of the fir-dev upstreaming patch, I'm not the primary author of this code so maybe @schweitz can comment on that.

I'm not the primary author of this code either.

The background here is that this is some prototype experimental work done to gauge the feasibility of using the affine dialect from Fortran. There are problems here as well as some bit rot. (The experiment yielded some promising results and discovered some interesting issues, as you might expect.)

@clementval is upstreaming it effectively in an "as-is" state, sharing it with a broader audience, and as a possible starting point for future work. Pros: It puts the prototype out there for anyone interested. Cons: It's just an early prototype. That's just one alternative.

Another alternative is to not bother upstreaming it, since it may be "too much of a prototype" (It's not always super clear how to distinguish "overly prototype-y" from "prototype-y but possibly useful in a new project.), and let someone figure out what to do in the future. Pros: It's not there and no one will mistakenly assume its robust, productized code by mistake. Cons: It's not there, so the reference point, lessons learned, etc. are likely to just be lost over time.

[schweitz]

We've made a decision a long time ago to use auto routinely and let the compiler type inference.

Here is an actual type from the front end that might help in understanding why fully elaborated types do not improve readability.

Understanding the code concerns can be addressed in more economical and truly effective ways.

std::__detail::__variant::__gen_vtable_impl<std::__detail::__variant::_Multi_array<void (*)(Fortran::common::visitors<auto Fortran::lower::pft::ReferenceVariantBase<true, Fortran::parser::AllocateStmt, Fortran::parser::AssignmentStmt, Fortran::parser::BackspaceStmt, Fortran::parser::CallStmt, Fortran::parser::CloseStmt, Fortran::parser::ContinueStmt, Fortran::parser::CycleStmt, Fortran::parser::DeallocateStmt, Fortran::parser::EndfileStmt, Fortran::parser::EventPostStmt, Fortran::parser::EventWaitStmt, Fortran::parser::ExitStmt, Fortran::parser::FailImageStmt, Fortran::parser::FlushStmt, Fortran::parser::FormTeamStmt, Fortran::parser::GotoStmt, Fortran::parser::IfStmt, Fortran::parser::InquireStmt, Fortran::parser::LockStmt, Fortran::parser::NullifyStmt, Fortran::parser::OpenStmt, Fortran::parser::PointerAssignmentStmt, Fortran::parser::PrintStmt, Fortran::parser::ReadStmt, Fortran::parser::ReturnStmt, Fortran::parser::RewindStmt, Fortran::parser::StopStmt, Fortran::parser::SyncAllStmt, Fortran::parser::SyncImagesStmt, Fortran::parser::SyncMemoryStmt, Fortran::parser::SyncTeamStmt, Fortran::parser::UnlockStmt, Fortran::parser::WaitStmt, Fortran::parser::WhereStmt, Fortran::parser::WriteStmt, Fortran::parser::ComputedGotoStmt, Fortran::parser::ForallStmt, Fortran::parser::ArithmeticIfStmt, Fortran::parser::AssignStmt, Fortran::parser::AssignedGotoStmt, Fortran::parser::PauseStmt, Fortran::parser::EntryStmt, Fortran::parser::FormatStmt, Fortran::parser::AssociateStmt, Fortran::parser::EndAssociateStmt, Fortran::parser::BlockStmt, Fortran::parser::EndBlockStmt, Fortran::parser::SelectCaseStmt, Fortran::parser::CaseStmt, Fortran::parser::EndSelectStmt, Fortran::parser::ChangeTeamStmt, Fortran::parser::EndChangeTeamStmt, Fortran::parser::CriticalStmt, Fortran::parser::EndCriticalStmt, Fortran::parser::NonLabelDoStmt, Fortran::parser::EndDoStmt, Fortran::parser::IfThenStmt, Fortran::parser::ElseIfStmt, Fortran::parser::ElseStmt, Fortran::parser::EndIfStmt, Fortran::parser::SelectRankStmt, Fortran::parser::SelectRankCaseStmt, Fortran::parser::SelectTypeStmt, Fortran::parser::TypeGuardStmt, Fortran::parser::WhereConstructStmt, Fortran::parser::MaskedElsewhereStmt, Fortran::parser::ElsewhereStmt, Fortran::parser::EndWhereStmt, Fortran::parser::ForallConstructStmt, Fortran::parser::EndForallStmt, Fortran::parser::EndProgramStmt, Fortran::parser::EndFunctionStmt, Fortran::parser::EndSubroutineStmt, Fortran::parser::EndMpSubprogramStmt, Fortran::parser::AssociateConstruct, Fortran::parser::BlockConstruct, Fortran::parser::CaseConstruct, Fortran::parser::ChangeTeamConstruct, Fortran::parser::CriticalConstruct, Fortran::parser::DoConstruct, Fortran::parser::IfConstruct, Fortran::parser::SelectRankConstruct, Fortran::parser::SelectTypeConstruct, Fortran::parser::WhereConstruct, Fortran::parser::ForallConstruct, Fortran::parser::CompilerDirective, Fortran::parser::OpenACCConstruct, Fortran::parser::OpenMPConstruct, Fortran::parser::OmpEndLoopDirective>::visit<(anonymous namespace)::FirConverter::genFIR(Fortran::lower::pft::Evaluation&, bool)::'lambda'(auto const&)>(auto&&) const::'lambda'(auto)>&&, std::variant<Fortran::common::Reference<Fortran::parser::AllocateStmt const>, Fortran::common::Reference<Fortran::parser::AssignmentStmt const>, Fortran::common::Reference<Fortran::parser::BackspaceStmt const>, Fortran::common::Reference<Fortran::parser::CallStmt const>, Fortran::common::Reference<Fortran::parser::CloseStmt const>, Fortran::common::Reference<Fortran::parser::ContinueStmt const>, Fortran::common::Reference<Fortran::parser::CycleStmt const>, Fortran::common::Reference<Fortran::parser::DeallocateStmt const>, Fortran::common::Reference<Fortran::parser::EndfileStmt const>, Fortran::common::Reference<Fortran::parser::EventPostStmt const>, Fortran::common::Reference<Fortran::parser::EventWaitStmt const>, Fortran::common::Reference<Fortran::parser::ExitStmt const>, Fortran::common::Reference<Fortran::parser::FailImageStmt const>, Fortran::common::Reference<Fortran::parser::FlushStmt const>, Fortran::common::Reference<Fortran::parser::FormTeamStmt const>, Fortran::common::Reference<Fortran::parser::GotoStmt const>, Fortran::common::Reference<Fortran::parser::IfStmt const>, Fortran::common::Reference<Fortran::parser::InquireStmt const>, Fortran::common::Reference<Fortran::parser::LockStmt const>, Fortran::common::Reference<Fortran::parser::NullifyStmt const>, Fortran::common::Reference<Fortran::parser::OpenStmt const>, Fortran::common::Reference<Fortran::parser::PointerAssignmentStmt const>, Fortran::common::Reference<Fortran::parser::PrintStmt const>, Fortran::common::Reference<Fortran::parser::ReadStmt const>, Fortran::common::Reference<Fortran::parser::ReturnStmt const>, Fortran::common::Reference<Fortran::parser::RewindStmt const>, Fortran::common::Reference<Fortran::parser::StopStmt const>, Fortran::common::Reference<Fortran::parser::SyncAllStmt const>, Fortran::common::Reference<Fortran::parser::SyncImagesStmt const>, Fortran::common::Reference<Fortran::parser::SyncMemoryStmt const>, Fortran::common::Reference<Fortran::parser::SyncTeamStmt const>, Fortran::common::Reference<Fortran::parser::UnlockStmt const>, Fortran::common::Reference<Fortran::parser::WaitStmt const>, Fortran::common::Reference<Fortran::parser::WhereStmt const>, Fortran::common::Reference<Fortran::parser::WriteStmt const>, Fortran::common::Reference<Fortran::parser::ComputedGotoStmt const>, Fortran::common::Reference<Fortran::parser::ForallStmt const>, Fortran::common::Reference<Fortran::parser::ArithmeticIfStmt const>, Fortran::common::Reference<Fortran::parser::AssignStmt const>, Fortran::common::Reference<Fortran::parser::AssignedGotoStmt const>, Fortran::common::Reference<Fortran::parser::PauseStmt const>, Fortran::common::Reference<Fortran::parser::EntryStmt const>, Fortran::common::Reference<Fortran::parser::FormatStmt const>, Fortran::common::Reference<Fortran::parser::AssociateStmt const>, Fortran::common::Reference<Fortran::parser::EndAssociateStmt const>, Fortran::common::Reference<Fortran::parser::BlockStmt const>, Fortran::common::Reference<Fortran::parser::EndBlockStmt const>, Fortran::common::Reference<Fortran::parser::SelectCaseStmt const>, Fortran::common::Reference<Fortran::parser::CaseStmt const>, Fortran::common::Reference<Fortran::parser::EndSelectStmt const>, Fortran::common::Reference<Fortran::parser::ChangeTeamStmt const>, Fortran::common::Reference<Fortran::parser::EndChangeTeamStmt const>, Fortran::common::Reference<Fortran::parser::CriticalStmt const>, Fortran::common::Reference<Fortran::parser::EndCriticalStmt const>, Fortran::common::Reference<Fortran::parser::NonLabelDoStmt const>, Fortran::common::Reference<Fortran::parser::EndDoStmt const>, Fortran::common::Reference<Fortran::parser::IfThenStmt const>, Fortran::common::Reference<Fortran::parser::ElseIfStmt const>, Fortran::common::Reference<Fortran::parser::ElseStmt const>, Fortran::common::Reference<Fortran::parser::EndIfStmt const>, Fortran::common::Reference<Fortran::parser::SelectRankStmt const>, Fortran::common::Reference<Fortran::parser::SelectRankCaseStmt const>, Fortran::common::Reference<Fortran::parser::SelectTypeStmt const>, Fortran::common::Reference<Fortran::parser::TypeGuardStmt const>, Fortran::common::Reference<Fortran::parser::WhereConstructStmt const>, Fortran::common::Reference<Fortran::parser::MaskedElsewhereStmt const>, Fortran::common::Reference<Fortran::parser::ElsewhereStmt const>, Fortran::common::Reference<Fortran::parser::EndWhereStmt const>, Fortran::common::Reference<Fortran::parser::ForallConstructStmt const>, Fortran::common::Reference<Fortran::parser::EndForallStmt const>, Fortran::common::Reference<Fortran::parser::EndProgramStmt const>, Fortran::common::Reference<Fortran::parser::EndFunctionStmt const>, Fortran::common::Reference<Fortran::parser::EndSubroutineStmt const>, Fortran::common::Reference<Fortran::parser::EndMpSubprogramStmt const>, Fortran::common::Reference<Fortran::parser::AssociateConstruct const>, Fortran::common::Reference<Fortran::parser::BlockConstruct const>, Fortran::common::Reference<Fortran::parser::CaseConstruct const>, Fortran::common::Reference<Fortran::parser::ChangeTeamConstruct const>, Fortran::common::Reference<Fortran::parser::CriticalConstruct const>, Fortran::common::Reference<Fortran::parser::DoConstruct const>, Fortran::common::Reference<Fortran::parser::IfConstruct const>, Fortran::common::Reference<Fortran::parser::SelectRankConstruct const>, Fortran::common::Reference<Fortran::parser::SelectTypeConstruct const>, Fortran::common::Reference<Fortran::parser::WhereConstruct const>, Fortran::common::Reference<Fortran::parser::ForallConstruct const>, Fortran::common::Reference<Fortran::parser::CompilerDirective const>, Fortran::common::Reference<Fortran::parser::OpenACCConstruct const>, Fortran::common::Reference<Fortran::parser::OpenMPConstruct const>, Fortran::common::Reference<Fortran::parser::OmpEndLoopDirective const> > const&)>, std::tuple<std::variant<Fortran::common::Reference<Fortran::parser::AllocateStmt const>, Fortran::common::Reference<Fortran::parser::AssignmentStmt const>, Fortran::common::Reference<Fortran::parser::BackspaceStmt const>, Fortran::common::Reference<Fortran::parser::CallStmt const>, Fortran::common::Reference<Fortran::parser::CloseStmt const>, Fortran::common::Reference<Fortran::parser::ContinueStmt const>, Fortran::common::Reference<Fortran::parser::CycleStmt const>, Fortran::common::Reference<Fortran::parser::DeallocateStmt const>, Fortran::common::Reference<Fortran::parser::EndfileStmt const>, Fortran::common::Reference<Fortran::parser::EventPostStmt const>, Fortran::common::Reference<Fortran::parser::EventWaitStmt const>, Fortran::common::Reference<Fortran::parser::ExitStmt const>, Fortran::common::Reference<Fortran::parser::FailImageStmt const>, Fortran::common::Reference<Fortran::parser::FlushStmt const>, Fortran::common::Reference<Fortran::parser::FormTeamStmt const>, Fortran::common::Reference<Fortran::parser::GotoStmt const>, Fortran::common::Reference<Fortran::parser::IfStmt const>, Fortran::common::Reference<Fortran::parser::InquireStmt const>, Fortran::common::Reference<Fortran::parser::LockStmt const>, Fortran::common::Reference<Fortran::parser::NullifyStmt const>, Fortran::common::Reference<Fortran::parser::OpenStmt const>, Fortran::common::Reference<Fortran::parser::PointerAssignmentStmt const>, Fortran::common::Reference<Fortran::parser::PrintStmt const>, Fortran::common::Reference<Fortran::parser::ReadStmt const>, Fortran::common::Reference<Fortran::parser::ReturnStmt const>, Fortran::common::Reference<Fortran::parser::RewindStmt const>, Fortran::common::Reference<Fortran::parser::StopStmt const>, Fortran::common::Reference<Fortran::parser::SyncAllStmt const>, Fortran::common::Reference<Fortran::parser::SyncImagesStmt const>, Fortran::common::Reference<Fortran::parser::SyncMemoryStmt const>, Fortran::common::Reference<Fortran::parser::SyncTeamStmt const>, Fortran::common::Reference<Fortran::parser::UnlockStmt const>, Fortran::common::Reference<Fortran::parser::WaitStmt const>, Fortran::common::Reference<Fortran::parser::WhereStmt const>, Fortran::common::Reference<Fortran::parser::WriteStmt const>, Fortran::common::Reference<Fortran::parser::ComputedGotoStmt const>, Fortran::common::Reference<Fortran::parser::ForallStmt const>, Fortran::common::Reference<Fortran::parser::ArithmeticIfStmt const>, Fortran::common::Reference<Fortran::parser::AssignStmt const>, Fortran::common::Reference<Fortran::parser::AssignedGotoStmt const>, Fortran::common::Reference<Fortran::parser::PauseStmt const>, Fortran::common::Reference<Fortran::parser::EntryStmt const>, Fortran::common::Reference<Fortran::parser::FormatStmt const>, Fortran::common::Reference<Fortran::parser::AssociateStmt const>, Fortran::common::Reference<Fortran::parser::EndAssociateStmt const>, Fortran::common::Reference<Fortran::parser::BlockStmt const>, Fortran::common::Reference<Fortran::parser::EndBlockStmt const>, Fortran::common::Reference<Fortran::parser::SelectCaseStmt const>, Fortran::common::Reference<Fortran::parser::CaseStmt const>, Fortran::common::Reference<Fortran::parser::EndSelectStmt const>, Fortran::common::Reference<Fortran::parser::ChangeTeamStmt const>, Fortran::common::Reference<Fortran::parser::EndChangeTeamStmt const>, Fortran::common::Reference<Fortran::parser::CriticalStmt const>, Fortran::common::Reference<Fortran::parser::EndCriticalStmt const>, Fortran::common::Reference<Fortran::parser::NonLabelDoStmt const>, Fortran::common::Reference<Fortran::parser::EndDoStmt const>, Fortran::common::Reference<Fortran::parser::IfThenStmt const>, Fortran::common::Reference<Fortran::parser::ElseIfStmt const>, Fortran::common::Reference<Fortran::parser::ElseStmt const>, Fortran::common::Reference<Fortran::parser::EndIfStmt const>, Fortran::common::Reference<Fortran::parser::SelectRankStmt const>, Fortran::common::Reference<Fortran::parser::SelectRankCaseStmt const>, Fortran::common::Reference<Fortran::parser::SelectTypeStmt const>, Fortran::common::Reference<Fortran::parser::TypeGuardStmt const>, Fortran::common::Reference<Fortran::parser::WhereConstructStmt const>, Fortran::common::Reference<Fortran::parser::MaskedElsewhereStmt const>, Fortran::common::Reference<Fortran::parser::ElsewhereStmt const>, Fortran::common::Reference<Fortran::parser::EndWhereStmt const>, Fortran::common::Reference<Fortran::parser::ForallConstructStmt const>, Fortran::common::Reference<Fortran::parser::EndForallStmt const>, Fortran::common::Reference<Fortran::parser::EndProgramStmt const>, Fortran::common::Reference<Fortran::parser::EndFunctionStmt const>, Fortran::common::Reference<Fortran::parser::EndSubroutineStmt const>, Fortran::common::Reference<Fortran::parser::EndMpSubprogramStmt const>, Fortran::common::Reference<Fortran::parser::AssociateConstruct const>, Fortran::common::Reference<Fortran::parser::BlockConstruct const>, Fortran::common::Reference<Fortran::parser::CaseConstruct const>, Fortran::common::Reference<Fortran::parser::ChangeTeamConstruct const>, Fortran::common::Reference<Fortran::parser::CriticalConstruct const>, Fortran::common::Reference<Fortran::parser::DoConstruct const>, Fortran::common::Reference<Fortran::parser::IfConstruct const>, Fortran::common::Reference<Fortran::parser::SelectRankConstruct const>, Fortran::common::Reference<Fortran::parser::SelectTypeConstruct const>, Fortran::common::Reference<Fortran::parser::WhereConstruct const>, Fortran::common::Reference<Fortran::parser::ForallConstruct const>, Fortran::common::Reference<Fortran::parser::CompilerDirective const>, Fortran::common::Reference<Fortran::parser::OpenACCConstruct const>, Fortran::common::Reference<Fortran::parser::OpenMPConstruct const>, Fortran::common::Reference<Fortran::parser::OmpEndLoopDirective const> > const&>, std::integer_sequence<unsigned long, 84ul> >::__visit_invoke(Fortran::common::visitors<auto Fortran::lower::pft::ReferenceVariantBase<true, Fortran::parser::AllocateStmt, Fortran::parser::AssignmentStmt, Fortran::parser::BackspaceStmt, Fortran::parser::CallStmt, Fortran::parser::CloseStmt, Fortran::parser::ContinueStmt, Fortran::parser::CycleStmt, Fortran::parser::DeallocateStmt, Fortran::parser::EndfileStmt, Fortran::parser::EventPostStmt, Fortran::parser::EventWaitStmt, Fortran::parser::ExitStmt, Fortran::parser::FailImageStmt, Fortran::parser::FlushStmt, Fortran::parser::FormTeamStmt, Fortran::parser::GotoStmt, Fortran::parser::IfStmt, Fortran::parser::InquireStmt, Fortran::parser::LockStmt, Fortran::parser::NullifyStmt, Fortran::parser::OpenStmt, Fortran::parser::PointerAssignmentStmt, Fortran::parser::PrintStmt, Fortran::parser::ReadStmt, Fortran::parser::ReturnStmt, Fortran::parser::RewindStmt, Fortran::parser::StopStmt, Fortran::parser::SyncAllStmt, Fortran::parser::SyncImagesStmt, Fortran::parser::SyncMemoryStmt, Fortran::parser::SyncTeamStmt, Fortran::parser::UnlockStmt, Fortran::parser::WaitStmt, Fortran::parser::WhereStmt, Fortran::parser::WriteStmt, Fortran::parser::ComputedGotoStmt, Fortran::parser::ForallStmt, Fortran::parser::ArithmeticIfStmt, Fortran::parser::AssignStmt, Fortran::parser::AssignedGotoStmt, Fortran::parser::PauseStmt, Fortran::parser::EntryStmt, Fortran::parser::FormatStmt, Fortran::parser::AssociateStmt, Fortran::parser::EndAssociateStmt, Fortran::parser::BlockStmt, Fortran::parser::EndBlockStmt, Fortran::parser::SelectCaseStmt, Fortran::parser::CaseStmt, Fortran::parser::EndSelectStmt, Fortran::parser::ChangeTeamStmt, Fortran::parser::EndChangeTeamStmt, Fortran::parser::CriticalStmt, Fortran::parser::EndCriticalStmt, Fortran::parser::NonLabelDoStmt, Fortran::parser::EndDoStmt, Fortran::parser::IfThenStmt, Fortran::parser::ElseIfStmt, Fortran::parser::ElseStmt, Fortran::parser::EndIfStmt, Fortran::parser::SelectRankStmt, Fortran::parser::SelectRankCaseStmt, Fortran::parser::SelectTypeStmt, Fortran::parser::TypeGuardStmt, Fortran::parser::WhereConstructStmt, Fortran::parser::MaskedElsewhereStmt, Fortran::parser::ElsewhereStmt, Fortran::parser::EndWhereStmt, Fortran::parser::ForallConstructStmt, Fortran::parser::EndForallStmt, Fortran::parser::EndProgramStmt, Fortran::parser::EndFunctionStmt, Fortran::parser::EndSubroutineStmt, Fortran::parser::EndMpSubprogramStmt, Fortran::parser::AssociateConstruct, Fortran::parser::BlockConstruct, Fortran::parser::CaseConstruct, Fortran::parser::ChangeTeamConstruct, Fortran::parser::CriticalConstruct, Fortran::parser::DoConstruct, Fortran::parser::IfConstruct, Fortran::parser::SelectRankConstruct, Fortran::parser::SelectTypeConstruct, Fortran::parser::WhereConstruct, Fortran::parser::ForallConstruct, Fortran::parser::CompilerDirective, Fortran::parser::OpenACCConstruct, Fortran::parser::OpenMPConstruct, Fortran::parser::OmpEndLoopDirective>::visit<(anonymous namespace)::FirConverter::genFIR(Fortran::lower::pft::Evaluation&, bool)::'lambda'(auto const&)>(auto&&) const::'lambda'(auto)>&&, std::variant<Fortran::common::Reference<Fortran::parser::AllocateStmt const>, Fortran::common::Reference<Fortran::parser::AssignmentStmt const>, Fortran::common::Reference<Fortran::parser::BackspaceStmt const>, Fortran::common::Reference<Fortran::parser::CallStmt const>, Fortran::common::Reference<Fortran::parser::CloseStmt const>, Fortran::common::Reference<Fortran::parser::ContinueStmt const>, Fortran::common::Reference<Fortran::parser::CycleStmt const>, Fortran::common::Reference<Fortran::parser::DeallocateStmt const>, Fortran::common::Reference<Fortran::parser::EndfileStmt const>, Fortran::common::Reference<Fortran::parser::EventPostStmt const>, Fortran::common::Reference<Fortran::parser::EventWaitStmt const>, Fortran::common::Reference<Fortran::parser::ExitStmt const>, Fortran::common::Reference<Fortran::parser::FailImageStmt const>, Fortran::common::Reference<Fortran::parser::FlushStmt const>, Fortran::common::Reference<Fortran::parser::FormTeamStmt const>, Fortran::common::Reference<Fortran::parser::GotoStmt const>, Fortran::common::Reference<Fortran::parser::IfStmt const>, Fortran::common::Reference<Fortran::parser::InquireStmt const>, Fortran::common::Reference<Fortran::parser::LockStmt const>, Fortran::common::Reference<Fortran::parser::NullifyStmt const>, Fortran::common::Reference<Fortran::parser::OpenStmt const>, Fortran::common::Reference<Fortran::parser::PointerAssignmentStmt const>, Fortran::common::Reference<Fortran::parser::PrintStmt const>, Fortran::common::Reference<Fortran::parser::ReadStmt const>, Fortran::common::Reference<Fortran::parser::ReturnStmt const>, Fortran::common::Reference<Fortran::parser::RewindStmt const>, Fortran::common::Reference<Fortran::parser::StopStmt const>, Fortran::common::Reference<Fortran::parser::SyncAllStmt const>, Fortran::common::Reference<Fortran::parser::SyncImagesStmt const>, Fortran::common::Reference<Fortran::parser::SyncMemoryStmt const>, Fortran::common::Reference<Fortran::parser::SyncTeamStmt const>, Fortran::common::Reference<Fortran::parser::UnlockStmt const>, Fortran::common::Reference<Fortran::parser::WaitStmt const>, Fortran::common::Reference<Fortran::parser::WhereStmt const>, Fortran::common::Reference<Fortran::parser::WriteStmt const>, Fortran::common::Reference<Fortran::parser::ComputedGotoStmt const>, Fortran::common::Reference<Fortran::parser::ForallStmt const>, Fortran::common::Reference<Fortran::parser::ArithmeticIfStmt const>, Fortran::common::Reference<Fortran::parser::AssignStmt const>, Fortran::common::Reference<Fortran::parser::AssignedGotoStmt const>, Fortran::common::Reference<Fortran::parser::PauseStmt const>, Fortran::common::Reference<Fortran::parser::EntryStmt const>, Fortran::common::Reference<Fortran::parser::FormatStmt const>, Fortran::common::Reference<Fortran::parser::AssociateStmt const>, Fortran::common::Reference<Fortran::parser::EndAssociateStmt const>, Fortran::common::Reference<Fortran::parser::BlockStmt const>, Fortran::common::Reference<Fortran::parser::EndBlockStmt const>, Fortran::common::Reference<Fortran::parser::SelectCaseStmt const>, Fortran::common::Reference<Fortran::parser::CaseStmt const>, Fortran::common::Reference<Fortran::parser::EndSelectStmt const>, Fortran::common::Reference<Fortran::parser::ChangeTeamStmt const>, Fortran::common::Reference<Fortran::parser::EndChangeTeamStmt const>, Fortran::common::Reference<Fortran::parser::CriticalStmt const>, Fortran::common::Reference<Fortran::parser::EndCriticalStmt const>, Fortran::common::Reference<Fortran::parser::NonLabelDoStmt const>, Fortran::common::Reference<Fortran::parser::EndDoStmt const>, Fortran::common::Reference<Fortran::parser::IfThenStmt const>, Fortran::common::Reference<Fortran::parser::ElseIfStmt const>, Fortran::common::Reference<Fortran::parser::ElseStmt const>, Fortran::common::Reference<Fortran::parser::EndIfStmt const>, Fortran::common::Reference<Fortran::parser::SelectRankStmt const>, Fortran::common::Reference<Fortran::parser::SelectRankCaseStmt const>, Fortran::common::Reference<Fortran::parser::SelectTypeStmt const>, Fortran::common::Reference<Fortran::parser::TypeGuardStmt const>, Fortran::common::Reference<Fortran::parser::WhereConstructStmt const>, Fortran::common::Reference<Fortran::parser::MaskedElsewhereStmt const>, Fortran::common::Reference<Fortran::parser::ElsewhereStmt const>, Fortran::common::Reference<Fortran::parser::EndWhereStmt const>, Fortran::common::Reference<Fortran::parser::ForallConstructStmt const>, Fortran::common::Reference<Fortran::parser::EndForallStmt const>, Fortran::common::Reference<Fortran::parser::EndProgramStmt const>, Fortran::common::Reference<Fortran::parser::EndFunctionStmt const>, Fortran::common::Reference<Fortran::parser::EndSubroutineStmt const>, Fortran::common::Reference<Fortran::parser::EndMpSubprogramStmt const>, Fortran::common::Reference<Fortran::parser::AssociateConstruct const>, Fortran::common::Reference<Fortran::parser::BlockConstruct const>, Fortran::common::Reference<Fortran::parser::CaseConstruct const>, Fortran::common::Reference<Fortran::parser::ChangeTeamConstruct const>, Fortran::common::Reference<Fortran::parser::CriticalConstruct const>, Fortran::common::Reference<Fortran::parser::DoConstruct const>, Fortran::common::Reference<Fortran::parser::IfConstruct const>, Fortran::common::Reference<Fortran::parser::SelectRankConstruct const>, Fortran::common::Reference<Fortran::parser::SelectTypeConstruct const>, Fortran::common::Reference<Fortran::parser::WhereConstruct const>, Fortran::common::Reference<Fortran::parser::ForallConstruct const>, Fortran::common::Reference<Fortran::parser::CompilerDirective const>, Fortran::common::Reference<Fortran::parser::OpenACCConstruct const>, Fortran::common::Reference<Fortran::parser::OpenMPConstruct const>, Fortran::common::Reference<Fortran::parser::OmpEndLoopDirective const> > const&)

[mehdi_amini]

In D111288#3052799, @schweitz wrote:

We've made a decision a long time ago to use auto routinely and let the compiler type inference.

Here is an actual type from the front end that might help in understanding why fully elaborated types do not improve readability.

Right, our coding guidelines are indeed recommending to use auto to improve readability, and the example of type you show here is exactly why we have this guideline: auto can greatly improve readability.

But there are plenty of cases where the improvement isn't clear to me, look at for example the case at hand here:

for (auto &loadOp : loadOps) {

vs:

for (ReadOp loadOp : loadOps) {

Is the auto improving readability here? If not why should we use it then?

In this particular case it is even worse: to me the auto is actively misleading:

1. the variable is named "loadOp" but isn't a LoadOp and is ReadOp instead.
2. The code takes a reference to the op, which makes me think it isn't an op since these are value-based class.

This is same for using auto op where we could read Operation *op, etc.

[jdoerfert]

In D111288#3052150, @schweitz wrote:

Another alternative is to not bother upstreaming it, since it may be "too much of a prototype" (It's not always super clear how to distinguish "overly prototype-y" from "prototype-y but possibly useful in a new project.), and let someone figure out what to do in the future. Pros: It's not there and no one will mistakenly assume its robust, productized code by mistake. Cons: It's not there, so the reference point, lessons learned, etc. are likely to just be lost over time.

I'm fine with it being there in a state like this but not without actual comments, in the code and commit message, telling people that this is not sound. Dropping some code that is known buggy without any form of documentation is not helpful for anyone.

[clementval]

In D111288#3053413, @jdoerfert wrote:

In D111288#3052150, @schweitz wrote:

Another alternative is to not bother upstreaming it, since it may be "too much of a prototype" (It's not always super clear how to distinguish "overly prototype-y" from "prototype-y but possibly useful in a new project.), and let someone figure out what to do in the future. Pros: It's not there and no one will mistakenly assume its robust, productized code by mistake. Cons: It's not there, so the reference point, lessons learned, etc. are likely to just be lost over time.

I'm fine with it being there in a state like this but not without actual comments, in the code and commit message, telling people that this is not sound. Dropping some code that is known buggy without any form of documentation is not helpful for anyone.

As Eric mentioned we are not the authors of this code and we have two options here:

1. We upstream the code in its current shape (we can add a comment mentioning that) so the broader community can start from it.
2. We do not upstream these transformations and any new effort will have to start from scratch on that.

[mehdi_amini]

I think @jdoerfert request to document more careful the state of the code and the caveat seem reasonable to me.
If you don't understand what is there enough to document it, then yeah maybe you shouldn't upstream it.

[jdoerfert]

In D111288#3053917, @mehdi_amini wrote:

I think @jdoerfert request to document more careful the state of the code and the caveat seem reasonable to me.
If you don't understand what is there enough to document it, then yeah maybe you shouldn't upstream it.

Here and for the other one. Put a warning in the commit message and file comment. In both cases list the things I said as potential problems. State it is a prototype and not necessarily production quality.
Then it's fine to be upstreamed.

[clementval]

In D111288#3053959, @jdoerfert wrote:

Here and for the other one. Put a warning in the commit message and file comment. In both cases list the things I said as potential problems. State it is a prototype and not necessarily production quality.
Then it's fine to be upstreamed.

Sure we can add these comments.

[clementval]

In D111288#3053917, @mehdi_amini wrote:

I think @jdoerfert request to document more careful the state of the code and the caveat seem reasonable to me.
If you don't understand what is there enough to document it, then yeah maybe you shouldn't upstream it.

It's not that we do not understand the code but more how much resource we can put to upstream it.

[clementval]

Rebase

[clementval]

Rebase

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

104

These tests looks like entirely auto-generated, what should we expect here?

Also the source of the test seems like taken directly from a real-world example, without any attempt to document or minimize: this can be a maintenance problem moving forward.

[clementval]

flang/test/Fir/memref-data-flow.fir

104

I heard that when you guys discussed the upstreaming of fir-dev it was advise to use to MLIR script to generate the check because the current checks were too small.

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

104

I'm not aware of this, we have documented guidelines though: https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices

[clementval]

flang/test/Fir/memref-data-flow.fir

104

Thanks for the guidelines link. I'll have to check what we will do in Flang because I now have contradictory information from two sides.

[awarzynski]

flang/test/Fir/memref-data-flow.fir

104

I heard that when you guys discussed the upstreaming of fir-dev it was advise to use to MLIR script to generate the check because the current checks were too small.

I think that quite a few separate discussions have happened in the last year or so. I was just a by-stander most of the time, so can't really comment. Also, I'm not aware of anything public that we could refer to here (e.g. an RFC or something similar). Perhaps this particular aspect has not been discussed? :)

@mehdi_amini,

These tests looks like entirely auto-generated, what should we expect here?

IIUC, you *are* in favor of auto-generated tests (e.g. with generate-test-checks.py), but would prefer them to be a bit refactored, right? Otherwise it's not really obvious "what" is being tested. Basically,

Tests should be minimal, and only check what is absolutely necessary.

@clementval The check lines could be reduced to only contain the key operations being tested and making sure the these are generated correctly.

this can be a maintenance problem moving forward

I agree. Any change in MLIR that causes these tests to fail could be non-trivial to triage. But I also think that it will be quite tricky to re-generate these tests from scratch. Perhaps we could make sure that:

• every test is documented (e.g. by making a descriptive function name that highlights "what" is being tested)
• check lines not strictly related to the functionality being tested are reduced/removed

This way we would make any potential triaging much easier for ourselves without having to re-write these cases.

This is just my 2p. WDYT?

[mehdi_amini]

. Perhaps we could make sure that:

• • every test is documented (e.g. by making a descriptive function name that highlights "what" is being tested)
• check lines not strictly related to the functionality being tested are reduced/removed

Right, that seems to go in the direction of the guidelines I posted I think :)

Thanks!

[clementval]

flang/test/Fir/memref-data-flow.fir

104

I fully understand your points @awarzynski and it makes sense to me. Once again I just want to point out that we need to find a trade off in the current upstreaming process.
I'll not be able to refactor/rework deeply every single patch I post. Keep in mind that every change done here must be sync back in fir-dev until we can discard it (tests are the easy one to sync). fir-dev is still evolving everyday and if we want to get rid of it one day we need to find the right pace to the upstreaming can catch up with the daily evolution.
This is not a comment specifically for this patch but rather a general one for the current process we are in. AFAIK, I'm the only person spending time upstreaming fir-dev at the moment.
In most cases I'm not the author of the code and even refactoring tests can take quite some times. Time that I'm happy to spend when we find flaw or blockers.
We agreed in the past that we will try our best to make small patches because a single push like it was done for MLIR or the Flang frontends was pushed back.
We might need a new discussion on the usptreaming process if we want it to be successful in a timely manner.

[kiranchandramohan]

flang/test/Fir/memref-data-flow.fir

104

Many thanks, @clementval for your efforts in restarting and making quite some progress with upstreaming. I am in favour of upstreaming fast so that we do not have to work across two repositories (llvm-project and fir-dev). For the OpenMP work, we face some difficulties working across the two repos.

I think we can consider,

1. Increasing the number of people working on upstreaming. It might be possible (subject to approval) for one of us at Arm to join the upstreaming effort (either in helping to write or fix tests or regular upstreaming work). Will this be acceptable?
2. Adding non-critical but essential changes as bugs/issues and come back to this once we have llvm-project and fir-dev on par.
3. Stopping fir-dev work once we reach full Fortran 95 support and everyone joins upstreaming for a couple of months.

[clementval]

flang/test/Fir/memref-data-flow.fir

104

A simplification of the first test could look like the following. Would that be ok?

// CHECK-LABEL:   func @_QPf1dc(
// CHECK-SAME:                  %[[VAL_0:.*]]: !fir.ref<!fir.array<60xi32>>, 
// CHECK:         ^bb2:
// CHECK:           %[[VAL_12:.*]] = fir.convert %{{.*}} : (index) -> i32
// CHECK-NOT:       fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
// CHECK-NOT:       %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
// CHECK:           %[[VAL_13:.*]] = fir.convert %[[VAL_12]] : (i32) -> i64

// CHECK:    %[[VAL_15:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
// CHECK:    %[[VAL_16:.*]] = fir.load %[[VAL_15]] : !fir.ref<i32>

// CHECK:    %[[VAL_18:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
// CHECK:    fir.store %{{.*}} to %[[VAL_18]] : !fir.ref<i32>

// CHECK:  ^bb3:
// CHECK:     %[[VAL_21:.*]] = fir.convert %{{.*}} : (index) -> i32
// CHECK-NOT: fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
// CHECK:     br ^bb4

// CHECK:  ^bb5:
// CHECK:    %[[VAL_25:.*]] = fir.convert %{{.*}} : (index) -> i32
// CHECK-NOT: fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
// CHECK-NOT: %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
// CHECK:    %[[VAL_26:.*]] = fir.convert %[[VAL_25]] : (i32) -> i64
// CHECK:    %[[VAL_28:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
// CHECK:    %[[VAL_29:.*]] = fir.load %[[VAL_28]] : !fir.ref<i32>
// CHECK:    %[[VAL_30:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
// CHECK:    %[[VAL_31:.*]] = fir.load %[[VAL_30]] : !fir.ref<i32>
// CHECK:    %[[VAL_33:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
// CHECK:    fir.store %{{.*}} to %[[VAL_33]] : !fir.ref<i32>

[clementval]

flang/test/Fir/memref-data-flow.fir

104

I think we can consider,

1. Increasing the number of people working on upstreaming. It might be possible (subject to approval) for one of us at Arm to join the upstreaming effort (either in helping to write or fix tests or regular upstreaming work). Will this be acceptable?

Of course more people working on this will help. We just need to coordinate if this is going to happen.

2. Adding non-critical but essential changes as bugs/issues and come back to this once we have llvm-project and fir-dev on par.

I think we should go this route. I would suggests that critical changes should be marked as "Request Changes" and any other comment would be done as best effort. Would this be acceptable? Also it doesn't mean that this comments will never be addressed but rather delayed until we can discard fir-dev.

3. Stopping fir-dev work once we reach full Fortran 95 support and everyone joins upstreaming for a couple of months.

I think we should at least reduce what we accept on fir-dev once F95 is reached. I guess we can talk about this during the next call.

[awarzynski]

flang/test/Fir/memref-data-flow.fir

108–163

Perhaps something like this would be sufficient? It highlights that:

• the store-load chains have been removed (as expected)
• basic blocks have been preserved (as expected)
• stores and loads that are not in a load-store chain are preserved

I also removed all regex regex variables. These didn't seem required.

This is not too different to what @clementval proposed, but leverages CHECK-LABEL and CHECK-DAG to make it a bit more robust.

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

104

I think we can defer some action items, but I would prefer that a bug is filed and mentioned in a comment like this strawman: // TODO: llvm.org/pr12345 this needs to be ....

We can either keep track of these with a "tag"/"keyword" on the bug, or with a separate "component" on buganizer.

[clementval]

flang/test/Fir/memref-data-flow.fir

104

Ok we will coordinate on this.

@mehdi_amini Is the smaller test proposed by me or @awarzynski look ok to you? If so, I can update this patch so we can move forward,

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

104

I am not sure: I haven't spent time on the existing test, but I'd start with the function name which isn't descriptive: can we name the function against what it is testing?
In some cases we may need comments about what is the test exercising as well.

I see tests very much as part of the documentation for the code: in many case when I'm unsure about some part of a pass, I'll try to disable it in the code and run the tests to see which case is breaking for example. But having over-constrained test or test where the input IR isn't minimized makes it impossible.

[clementval]

Reduce number of fir-to-fir tests and reduce the checks

[clementval]

flang/test/Fir/memref-data-flow.fir

104

@awarzynski @kiranchandramohan @mehdi_amini Test have been reduced and checks are just checking what is important.

[kiranchandramohan]

I guess some of the arithmetic operations at various places can be removed to shorten the test. Like for e.g:

%65 = arith.subi %64, %c1 : index
%66 = arith.muli %5, %65 : index
%67 = arith.addi %66, %62 : index

I am OK with creating a ticket to reduce the test later and proceeding. Please wait for Mehdi.

[kiranchandramohan]

flang/test/Fir/memref-data-flow.fir

404

CHECL-NOT -> CHECK-NOT and couple of places below.

[awarzynski]

Thanks for trimming the tests @clementval - now it is much clearer what is being tested! I think the the following pattern:

// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>

can be reduced to:

// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>

Not a blocker for me. I've also left a few inline comments, but mostly nits! LGTM (modulo the typos), but please wait for Mehdi to take a look before merging.

Thanks for working on this!

flang/test/Fir/memref-data-flow.fir

58

[nit] This comment applies to all cases in this file - I'd move it to the very top (near the RUN line). You may also want to note that fir.stores and fir.loads that are not in a chain are preserved. Perhaps that's obvious?

76–77

Just a [nit]

143–144

To make sure that there's no fir.store before fir.load

253

258–261

Just a [nit]

270–273

401

To make sure that there's no fir.store before fir.load.

427–430

[nit]

[clementval]

Fix typos and nits

[clementval]

In D111288#3069662, @kiranchandramohan wrote:

I guess some of the arithmetic operations at various places can be removed to shorten the test. Like for e.g:

%65 = arith.subi %64, %c1 : index
%66 = arith.muli %5, %65 : index
%67 = arith.addi %66, %62 : index

Thanks for the review. I'm not sure we should trim the inputs. At least not before we have lowering tests so we make sure dialects are good together.

flang/test/Fir/memref-data-flow.fir

404

Thanks for catching this.

[clementval]

In D111288#3069727, @awarzynski wrote:

Thanks for trimming the tests @clementval - now it is much clearer what is being tested! I think the the following pattern:

// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>

can be reduced to:

// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>

Not a blocker for me. I've also left a few inline comments, but mostly nits! LGTM (modulo the typos), but please wait for Mehdi to take a look before merging.

Thanks for working on this!

Thanks for the review. I made a last pass on this patch and hopefully the last one.

[clementval]

Move comment

[clementval]

Fix check typo

[clementval]

Can we go ahead we this?

[mehdi_amini]

Seems like I didn't click "send" when I reviewed this earlier this week, sorry!

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

This is marked as done, but it isn't (it isn't the only one I think)

flang/test/Fir/memref-data-flow.fir

62

Are we not expecting a fir.store? If so the check-not is more robust with simply // CHECK-NOT: fir.store

If you need to match the type as well I don't think you need to match the " to ", so // CHECK-NOT: fir.store{{.*}}!fir.ref<i32>

Same logic everywhere...

63

The two CHECK-NOT above apply to bb1, but in the source it is:

^bb1(%2: index, %3: index):  // 2 preds: ^bb0, ^bb2
  %4 = arith.cmpi sgt, %3, %c0 : index
  cond_br %4, ^bb2, ^bb3

Why are we specifically checking for no load/store in here?

66–67

I really don't understand this sequence: we have a CHECK-NOT followed by the identical CHECK.

74

This check-not is also followed by some identical checks, again the whole logic behind these checks is beyond me right now.

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

277

I still believe we should have documentation for the test itself: what is this IR exposing as a pattern and what it this testing that the other above don't?

This is quite a long function right now...

[awarzynski]

flang/test/Fir/memref-data-flow.fir

63

Currently, fir-memref-dataflow-opt makes sure that:

// All store-load chains are removed

These particular CHECK-NOTs verify that there are no new store-loads in bb1. There shouldn't be. It's just a sanity check in case something goes horribly wrong. Do you think that it's confusing?

66–67

IIUC, the first CHECK-NOT can safely be removed without affecting the test. From https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-not-directive :

The “CHECK-NOT:” directive is used to verify that a string doesn’t occur between two matches (or before the first match, or after the last match).

I also find it confusing, but I believe that it's valid and that it does check that there's no store-load chain here.

277

what it this testing that the other above don't

IIUC, it is similar to the other tests. I don't think there's anything particular here beyond: there are some store-load chains, which are being removed. Wouldn't that be sufficient?

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

I have discussed this with others and they would like to keep it like that. The guidlines are kind of blurry on this... If this is a blocker we will ave to discuss this further.

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

Let's discuss then.
(and please don't ignore comments or mark them as done when they aren't!)

flang/test/Fir/memref-data-flow.fir

66–67

If I had to document your check right now it would be (as I understand them):

// Check that there is at least one fir.load followed by at least one fir.store, 
// both on i32. The load/store don't have to be related to each other.

And I could write the exact same test with:

// CHECK-LABEL: ^bb2:
// CHECK:     fir.load{{.*}} !fir.ref<i32>
// CHECK:     fir.store{{.*} }!fir.ref<i32>

Do you see an input to FileCheck that would be handled differently by the code you have right now and the above?

Also please provide the kind of plain text english documentation I wrote above on what you're checking in each section.

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

Sorry for that. We have discussed in this patch or in maybe another one and in the last call that during the upstreaming work any "blocker" comment should be put as "Change Requested" so that we can identified where to put our effort.
In this particular case auto can surely be removed but not everywhere like you mentioned in your comment. I think that's more the everywhere that others were against.

auto does not help readability so why not spell out the type? (here and everywhere, in all the upstreaming patches please).

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

On this particular comment, I remember what happened. I did the change and marked it as done then after discussion I revrted the change but I forgot to mention that. Sorry for that, I didn't mean to discard your comment.

[clementval]

flang/test/Fir/memref-data-flow.fir

66–67

In the initial code we have two sequences of load/store. We want to make sure only one stays. Maybe using

// Verify that all store-load pairs have been eliminated. Other stores and loads are preserved.
CHECK-COUNT-1: fir.store
CHECK-COUNT-1: fir.load

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

66–67

That would make more sense to me.

[clementval]

Update the tests to make them easier to understand.

flang/test/Fir/memref-data-flow.fir

74

We check that the fir.store/fir.load pair has been removed here (between the fir.convert ops). The next checks make sure the loads have been preserved later. I added a comment to make it clearer.

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

@mehdi_amini We had discussion with the team and they mentioned couple of points why they would like to keep auto:

• It avoids to introduce type's bugs like

auto ch = getc(file);
changed to
char ch = getc(file);
it's a small type change. The char type looks like documentation but it is actually introducing a bug that might be really hard to detect.

• Also refactoring is easier if we have type inference.

Not sure if this is addressing your comment here. If not please let us know and we can discuss further how we can best approach this since it will be a recurrent pattern in the Flang code.

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

These are very valid arguments, but they challenge the documented status quo in LLVM. As such I don't think this is something that can just be settled here in a revision.

I invite you to send an RFC (and a patch) to llvm-dev@ to update the coding standards if this is something that "the team" really believe is important to change: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

flang/test/Fir/memref-data-flow.fir

277

Wouldn't that be sufficient?

Not really: this isn't really a good way to write and design tests IMO.

Ideally I'd like the tests to look like this for example: https://github.com/llvm/llvm-project/blob/main/mlir/test/Transforms/loop-coalescing.mlir
Note how every individual case is shared in a different function, and how documented they are in what they are actually testing.

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

277

(You can browse all the tests in this directory, or in MLIR in general, they should mostly look like this or have a similar overall form. I don't think you'll find any comparable to what is here)

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

85

I'm also interested in what "the team" would argue here: this code is very misleading in the naming and this is mostly because of the use of auto.

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

85

In this case ReadOp is a template parameter so not sure it would help here. This is what I gathered from the team so if we need to discuss this in more details we can arrange something.

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

85

OK I totally missed that this was a template parameter in my initial comment here!

By the way there seems to be a single instantiation of the template right now? It this intended to have more?

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

85

I can find only a single instantiation of the template in fir-dev right now.

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

We have talked together and we do not think the main LLVM guideline should be change for that. We will update couple of autos in the upstreaming patch where it makes sense.
We will also put up for review a update on the Flang guidelines like you have for MLIR (https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide) where we can precise the auto usage. Would that work for you?

I'm gonna update this patch and the other that is under review.

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

Note that the only place where MLIR deviates from LLVM really is the camelBack aspect (the other items are just things that LLVM does not define or aren't applicable in LLVM), and that wasn't intentional to deviate from LLVM: at the time there was a push in LLVM to adopt camelBack and we used it in anticipation. Had we known that LLVM would end-up not changing, we likely would have followed LLVM convention as-is.

Looking forward to the proposal for flang, I hope it won't go agains the LLVM style guide!

[clementval]

Update auto in some non evident places

[clementval]

Updated some of the non-evident auto usage.

[schweitz]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

Mehdi:

Would you help clarify direction here? Are you suggesting that you would be ok with revising the LLVM style just for the flang directories in question (just like MLIR did with respect to camelBack)? Or do you prefer that this be addressed for the entire llvm-project tree?

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

Unless there is a really good reason to, I don't think the flang directory is supposed to diverge from LLVM.
I understand the status quo (which was discussed on llvm-dev@ before deciding on integrating flang I believe) to be that we'll apply the existing LLVM coding standard (I remember that C++17 was an exception for example) and usual practices.

So I see two possible ways if you want to have flang code not follow the LLVM practices:

• renegotiate the status quo (I would go back to llvm-dev@ for this, since this is where this was agree'd upon before).
• update the LLVM-wide coding standards.

In general I rather keep consistency in the project, unless the specificities of a particular project are making a difference (for example libcxx is very constrained by the standard, the compiler-rt have "embedded" constraints, MLIR python bindings need exception handling, etc.)

[awarzynski]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

I believe that this is the most recent discussion on the usage of auto on llvm-dev: https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html. Based on that, an update to the LLVM-wide coding standards is likely to be a hard sell.

Perhaps the focus should be on "why" would Flang benefit from a bit more liberal approach here?

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

Once again, I don't think we do not follow the LLVM guidelines with auto. We have eased our stance here by updating auto where requested and we will do so future patches as well.
I don't think Eric is proposing a divergence from the guidelines at all but rather a clarification for Flang for a guideline that is subject to interpretation.

[schweitz]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

This concerns a very limited, narrow scope: local variable declarations with initialization expressions. The phrase "already obvious" in the coding conventions is ambiguous, an invitation to disagreement.

The use of auto allows the compiler to perform type inferencing on variables, resulting in the following benefits:

• Extremely long and complicated type names are not used, which is less error-prone and greatly improves understandability.
• The clutter from repetitive namespace tags is removed.
• Explicitly declared types can and do introduce hard-to-find bugs from unexpected or incorrect truncations, extensions, and other conversions.
• Using auto instead of declared types simplifies refactoring when interfaces are rapidly evolving.
• auto can be required when writing some templatized code.

[mehdi_amini]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

Right, what you write here seems reasonable. But you have to see how it plays in practice, and the 2 answers in the thread above are a good reference.

Also, if we come back at that case at hand here:

for (auto *user : v.getUsers())

vs

for (mlir::Operation *user : v.getUsers())

I don't think this is an "Extremely long and complicated type name" case, the namespace is annoying, what I'm doing in every project downstream (like TensorFlow) is to have a namespace inside the mlir namespace (mlir::tensorflow::). If your code is implemented inside this namespace, you remove all of the prefixes.

Explicitly declared types can and do introduce hard-to-find bugs from unexpected or incorrect truncations, extensions, and other conversions.

It's be great to have a very clear guideline on the kind of API that is subject to these problems, any reference by the way?

Using auto instead of declared types simplifies refactoring when interfaces are rapidly evolving.

Yes, but this is also a double-edged sword (code may compile when you wouldn't expect it possibly). Also, this isn't a rapidly evolving API here.

For this particular getUsers() API, it isn't defined in this file, so I wouldn't say that the context is obvious here to just know the type: having to lookup an API on another class isn't the current context to me.
And in this particular example it is worse because there is a very close API: v.getUses() (it differs by one letter!).
From experience, this users/uses is also a frequent source of confusion for users.

[clementval]

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

30

for (auto *user : v.getUsers())

Is a good example where we changed to

for (mlir::Operation *user : v.getUsers())

as requested.

[clementval]

This patch was on hold because of th auto discussion. I think we have solved this and we are aligned with the guidelines now. I'll wait until tomorrow and land it.

[mehdi_amini]

The test is still not really in a satisfying shape, could you rework this?

[clementval]

In D111288#3140668, @mehdi_amini wrote:

The test is still not really in a satisfying shape, could you rework this?

We trimmed the checks to a minimum and added explanatory comments. What else do you want to be changed?

[mehdi_amini]

flang/test/Fir/memref-data-flow.fir

83

What kind of pattern this function is exercising that isn't exercised by other tests?

I don't know what forward_store0 is covering compared to forward_store1...

(same question for forward_store2, forward_store3...)

156

This sequence of checks is fairly obscure to me.

(same below)

[clementval]

@mehdi_amini Ok we will work on that.

[clementval]

Remove tests checking similar behavior. Add comment in checks.

flang/include/flang/Optimizer/Transforms/Passes.h

 // Passes defined in Passes.td
 //===----------------------------------------------------------------------===//
 
 std::unique_ptr<mlir::Pass> createAbstractResultOptPass();
 std::unique_ptr<mlir::Pass> createAffineDemotionPass();
 std::unique_ptr<mlir::Pass> createFirToCfgPass();
 std::unique_ptr<mlir::Pass> createCharacterConversionPass();
 std::unique_ptr<mlir::Pass> createExternalNameConversionPass();
+std::unique_ptr<mlir::Pass> createMemDataFlowOptPass();
 std::unique_ptr<mlir::Pass> createPromoteToAffinePass();
 
 /// Support for inlining on FIR.
 bool canLegallyInline(mlir::Operation *op, mlir::Region *reg,
                       mlir::BlockAndValueMapping &map);
 
 // declarative passes
 #define GEN_PASS_REGISTRATION
 #include "flang/Optimizer/Transforms/Passes.h.inc"
 
 } // namespace fir
 
 #endif // OPTIMIZER_TRANSFORMS_PASSES_H

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()";
 }
 
+def MemRefDataFlowOpt : FunctionPass<"fir-memref-dataflow-opt"> {
+  let summary =
+    "Perform store/load forwarding and potentially removing dead stores.";
+  let description = [{
+    This pass performs store to load forwarding to eliminate memory accesses and
+    potentially the entire allocation if all the accesses are forwarded.
+  }];
+  let constructor = "::fir::createMemDataFlowOptPass()";
+  let dependentDialects = [
+    "fir::FIROpsDialect", "mlir::StandardOpsDialect"
+  ];
+}
+
 #endif // FLANG_OPTIMIZER_TRANSFORMS_PASSES

flang/lib/Optimizer/Transforms/CMakeLists.txt

 add_flang_library(FIRTransforms
   AbstractResult.cpp
   AffinePromotion.cpp
   AffineDemotion.cpp
   CharacterConversion.cpp
   Inliner.cpp
   ExternalNameConversion.cpp
+  MemRefDataFlowOpt.cpp
   RewriteLoop.cpp
 
   DEPENDS
   FIRDialect
   FIRSupport
   FIROptTransformsPassIncGen
 
   LINK_LIBS
   FIRDialect
   MLIRAffineToStandard
   MLIRLLVMIR
   MLIROpenACC
   MLIROpenMP
   FIRSupport
 )

flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp

+//===- MemRefDataFlowOpt.cpp - Memory DataFlow Optimization pass ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "PassDetail.h"
+#include "flang/Optimizer/Dialect/FIRDialect.h"
+#include "flang/Optimizer/Dialect/FIROps.h"
+#include "flang/Optimizer/Dialect/FIRType.h"
+#include "flang/Optimizer/Transforms/Passes.h"
+#include "mlir/Dialect/StandardOps/IR/Ops.h"
+#include "mlir/IR/Dominance.h"
+#include "mlir/IR/Operation.h"
+#include "mlir/Transforms/Passes.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+
+#define DEBUG_TYPE "fir-memref-dataflow-opt"
+
+namespace {
+
+template <typename OpT>
+static std::vector<OpT> getSpecificUsers(mlir::Value v) {
+  std::vector<OpT> ops;
+  for (mlir::Operation *user : v.getUsers())
+    if (auto op = dyn_cast<OpT>(user))
+      ops.push_back(op);
+  return ops;
+}
+
+/// This is based on MLIR's MemRefDataFlowOpt which is specialized on AffineRead
+/// and AffineWrite interface
+template <typename ReadOp, typename WriteOp>
+class LoadStoreForwarding {
+public:
+  LoadStoreForwarding(mlir::DominanceInfo *di) : domInfo(di) {}
+
+  // FIXME: This algorithm has a bug. It ignores escaping references between a
+  // store and a load.
+  llvm::Optional<WriteOp> findStoreToForward(ReadOp loadOp,
+                                             std::vector<WriteOp> &&storeOps) {
+    llvm::SmallVector<WriteOp> candidateSet;
+
+    for (auto storeOp : storeOps)
+      if (domInfo->dominates(storeOp, loadOp))
+        candidateSet.push_back(storeOp);
+
+    if (candidateSet.empty())
+      return {};
+
+    llvm::Optional<WriteOp> nearestStore;
+    for (auto candidate : candidateSet) {
+      auto nearerThan = [&](WriteOp otherStore) {
+        if (candidate == otherStore)
+          return false;
+        bool rv = domInfo->properlyDominates(candidate, otherStore);
+        if (rv) {
+          LLVM_DEBUG(llvm::dbgs()
+                     << "candidate " << candidate << " is not the nearest to "
+                     << loadOp << " because " << otherStore << " is closer\n");
+        }
+        return rv;
+      };
+      if (!llvm::any_of(candidateSet, nearerThan)) {
+        nearestStore = mlir::cast<WriteOp>(candidate);
+        break;
+      }
+    }
+    if (!nearestStore) {
+      LLVM_DEBUG(
+          llvm::dbgs()
+          << "load " << loadOp << " has " << candidateSet.size()
+          << " store candidates, but this algorithm can't find a best.\n");
+    }
+    return nearestStore;
+  }
+
+  llvm::Optional<ReadOp> findReadForWrite(WriteOp storeOp,
+                                          std::vector<ReadOp> &&loadOps) {
+    for (auto &loadOp : loadOps) {
+      if (domInfo->dominates(storeOp, loadOp))
+        return loadOp;
+    }
+    return {};
+  }
+
+private:
+  mlir::DominanceInfo *domInfo;
+};
+
+class MemDataFlowOpt : public fir::MemRefDataFlowOptBase<MemDataFlowOpt> {
+public:
+  void runOnFunction() override {
+    mlir::FuncOp f = getFunction();
+
+    auto *domInfo = &getAnalysis<mlir::DominanceInfo>();
+    LoadStoreForwarding<fir::LoadOp, fir::StoreOp> lsf(domInfo);
+    f.walk([&](fir::LoadOp loadOp) {
+      auto maybeStore = lsf.findStoreToForward(
+          loadOp, getSpecificUsers<fir::StoreOp>(loadOp.memref()));
+      if (maybeStore) {
+        auto storeOp = maybeStore.getValue();
+        LLVM_DEBUG(llvm::dbgs() << "FlangMemDataFlowOpt: In " << f.getName()
+                                << " erasing load " << loadOp
+                                << " with value from " << storeOp << '\n');
+        loadOp.getResult().replaceAllUsesWith(storeOp.value());
+        loadOp.erase();
+      }
+    });
+    f.walk([&](fir::AllocaOp alloca) {
+      for (auto &storeOp : getSpecificUsers<fir::StoreOp>(alloca.getResult())) {
+        if (!lsf.findReadForWrite(
+                storeOp, getSpecificUsers<fir::LoadOp>(storeOp.memref()))) {
+          LLVM_DEBUG(llvm::dbgs() << "FlangMemDataFlowOpt: In " << f.getName()
+                                  << " erasing store " << storeOp << '\n');
+          storeOp.erase();
+        }
+      }
+    });
+  }
+};
+} // namespace
+
+std::unique_ptr<mlir::Pass> fir::createMemDataFlowOptPass() {
+  return std::make_unique<MemDataFlowOpt>();
+}

flang/test/Fir/memref-data-flow.fir

+// RUN: fir-opt --split-input-file --fir-memref-dataflow-opt %s | FileCheck %s
+
+// Test that all load-store chains are removed
+
+func @load_store_chain_removal(%arg0: !fir.ref<!fir.array<60xi32>>, %arg1: !fir.ref<!fir.array<60xi32>>, %arg2: !fir.ref<!fir.array<60xi32>>) {
+  %c1_i64 = arith.constant 1 : i64
+  %c60 = arith.constant 60 : index
+  %c0 = arith.constant 0 : index
+  %c1 = arith.constant 1 : index
+  %0 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFf1dcEi"}
+  %1 = fir.alloca !fir.array<60xi32> {bindc_name = "t1", uniq_name = "_QFf1dcEt1"}
+  br ^bb1(%c1, %c60 : index, index)
+^bb1(%2: index, %3: index):  // 2 preds: ^bb0, ^bb2
+  %4 = arith.cmpi sgt, %3, %c0 : index
+  cond_br %4, ^bb2, ^bb3
+^bb2:  // pred: ^bb1
+  %5 = fir.convert %2 : (index) -> i32
+  fir.store %5 to %0 : !fir.ref<i32>
+  %6 = fir.load %0 : !fir.ref<i32>
+  %7 = fir.convert %6 : (i32) -> i64
+  %8 = arith.subi %7, %c1_i64 : i64
+  %9 = fir.coordinate_of %arg0, %8 : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
+  %10 = fir.load %9 : !fir.ref<i32>
+  %11 = arith.addi %10, %10 : i32
+  %12 = fir.coordinate_of %1, %8 : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
+  fir.store %11 to %12 : !fir.ref<i32>
+  %13 = arith.addi %2, %c1 : index
+  %14 = arith.subi %3, %c1 : index
+  br ^bb1(%13, %14 : index, index)
+^bb3:  // pred: ^bb1
+  %15 = fir.convert %2 : (index) -> i32
+  fir.store %15 to %0 : !fir.ref<i32>
+  br ^bb4(%c1, %c60 : index, index)
+^bb4(%16: index, %17: index):  // 2 preds: ^bb3, ^bb5
+  %18 = arith.cmpi sgt, %17, %c0 : index
+  cond_br %18, ^bb5, ^bb6
+^bb5:  // pred: ^bb4
+  %19 = fir.convert %16 : (index) -> i32
+  fir.store %19 to %0 : !fir.ref<i32>
+  %20 = fir.load %0 : !fir.ref<i32>
+  %21 = fir.convert %20 : (i32) -> i64
+  %22 = arith.subi %21, %c1_i64 : i64
+  %23 = fir.coordinate_of %1, %22 : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
+  %24 = fir.load %23 : !fir.ref<i32>
+  %25 = fir.coordinate_of %arg1, %22 : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
+  %26 = fir.load %25 : !fir.ref<i32>
+  %27 = arith.muli %24, %26 : i32
+  %28 = fir.coordinate_of %arg2, %22 : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
+  fir.store %27 to %28 : !fir.ref<i32>
+  %29 = arith.addi %16, %c1 : index
+  %30 = arith.subi %17, %c1 : index
+  br ^bb4(%29, %30 : index, index)
+^bb6:  // pred: ^bb4
+  %31 = fir.convert %16 : (index) -> i32
+  fir.store %31 to %0 : !fir.ref<i32>
+  return
+}
+
+// CHECK-LABEL: func @load_store_chain_removal
+// CHECK-LABEL: ^bb1
+// CHECK-LABEL: ^bb2:
+// Make sure the previous fir.store/fir.load pair have been elimated and we 
+// preserve the last pair of fir.load/fir.store.
+// CHECK-COUNT-1: %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK-COUNT-1: fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+// CHECK-LABEL: ^bb3:
+// Make sure the fir.store has been removed.
+// CHECK-NOT:     fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+// CHECK-LABEL: ^bb5:
+// CHECK:         %{{.*}} = fir.convert %{{.*}} : (index) -> i32
+// Check that the fir.store/fir.load pair has been removed between the convert.
+// CHECK-NOT:     fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.convert %{{.*}} : (i32) -> i64
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+// CHECK-LABEL: ^bb6:
+// CHECK-NOT:     fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>