Page MenuHomePhabricator

[mlir] NFC: Rename LoopOps dialect to SCF (Structured Control Flow)
ClosedPublic

Authored by ftynse on Thu, May 7, 7:58 AM.

Details

Summary

This dialect contains various structured control flow operaitons, not only
loops, reflect this in the name. Drop the Ops suffix for consistency with other
dialects.

Note that this only moves the files and changes the C++ namespace from 'loop'
to 'scf'. The visible IR prefix remains the same and will be updated
separately. The conversions will also be updated separately.

Diff Detail

Event Timeline

ftynse created this revision.Thu, May 7, 7:58 AM
antiagainst accepted this revision.Thu, May 7, 10:56 AM
This revision is now accepted and ready to land.Thu, May 7, 10:56 AM
This revision was automatically updated to reflect the committed changes.

@ftynse and @antiagainst , note that one of the clients of mlir is the flang project. Since this revision changed the interface of mlir, all clients of mlir should be checked to see if they still build and test correctly before delivering such changes. Note that the person who knows most about flang's dependency on mlir is @schweitz, but any member of the flang team should be able to verify if a change works for flang.

@ftynse and @antiagainst , note that one of the clients of mlir is the flang project. Since this revision changed the interface of mlir, all clients of mlir should be checked to see if they still build and test correctly before delivering such changes. Note that the person who knows most about flang's dependency on mlir is @schweitz, but any member of the flang team should be able to verify if a change works for flang.

I'm sorry this broke your build, I had approved the fix that was mailed before this message.

I don't know who are the members of the flang team (is there a list?) and would feel concerned if we need to block any changes to MLIR APIs for review by people other than regular MLIR reviewers. If something is broken by an identifiable commit, and the fix isn't obvious, the usual policy in LLVM is to revert and report the problem to the author of the commit. Sometimes, it may be faster to just write the author a quick direct email. Generally, this is something that can and should be solved with a pre-merge check, but the one we have is broken for random reasons so often that it becomes useless.

I don't know who are the members of the flang team (is there a list?) and would feel concerned if we need to block any changes to MLIR APIs for review by people other than regular MLIR reviewers.

I don't think this is the request. The request is just "don't break the build".
The pre-merge testing will test flang as well, but you would have had to rebase the patch after you landed the dependent ones first so that the pre-merge testing could work (I think marking the dependencies in Phabricator would have worked also).

In general trying to get the pre-merge testing green before pushing changes is safer.

I don't think this is the request. The request is just "don't break the build".
The pre-merge testing will test flang as well, but you would have had to rebase the patch after you landed the dependent ones first so that the pre-merge testing could work (I think marking the dependencies in Phabricator would have worked also).

In general trying to get the pre-merge testing green before pushing changes is safer.

Yes, the request is to not break the build. And in the case where the pre-merge testing is not working, I encourage you to contact @schweitz or @PeteSteinfeld or any other member of the flang team to verify that our builds succeed.

I don't think this is the request. The request is just "don't break the build".
The pre-merge testing will test flang as well, but you would have had to rebase the patch after you landed the dependent ones first so that the pre-merge testing could work (I think marking the dependencies in Phabricator would have worked also).

In general trying to get the pre-merge testing green before pushing changes is safer.

Yes, the request is to not break the build.

This is standard policy. So is reverting the breaking changes. Sometimes we do break stuff and there are technical solutions to decrease the probability of this happening.

I am however opposed to having to contact someone other than people who would normally review the patch. If something is not covered by check-flang (which I forgot to run, I admit, that's why we need automation!) or any other simple command, I think it is unreasonable to block us from working and wait for somebody to think about how the changes might affect dependent projects.

And in the case where the pre-merge testing is not working, I encourage you to contact @schweitz or @PeteSteinfeld or any other member of the flang team to verify that our builds succeed.

Thanks for offering help! Note that I still don't know who "any other member"s are, is there a list? a chat channel?

Thanks for offering help! Note that I still don't know who "any other member"s are, is there a list? a chat channel?

Thanks, @ftynse. It would have been sufficient to run check-flang.

There's a Slack channel for Fortran development at https://flang-compiler.slack.com/. There's also a mailing list at https://lists.llvm.org/pipermail/flang-dev/.

Pete

Thanks for offering help! Note that I still don't know who "any other member"s are, is there a list? a chat channel?

Thanks, @ftynse. It would have been sufficient to run check-flang.

Great! I'll just make an alias that runs both mlir and flang tests.

There's a Slack channel for Fortran development at https://flang-compiler.slack.com/. There's also a mailing list at https://lists.llvm.org/pipermail/flang-dev/.

Thanks for the information!

I don't think this is the request. The request is just "don't break the build".
The pre-merge testing will test flang as well, but you would have had to rebase the patch after you landed the dependent ones first so that the pre-merge testing could work (I think marking the dependencies in Phabricator would have worked also).

In general trying to get the pre-merge testing green before pushing changes is safer.

Yes, the request is to not break the build.

This is standard policy. So is reverting the breaking changes. Sometimes we do break stuff and there are technical solutions to decrease the probability of this happening.

I am however opposed to having to contact someone other than people who would normally review the patch. If something is not covered by check-flang (which I forgot to run, I admit, that's why we need automation!) or any other simple command, I think it is unreasonable to block us from working and wait for somebody to think about how the changes might affect dependent projects.

This is not what was asked: it was mentioned that some people can *help*, I think if you can't figure out yourself how to not break flang or if you don't understand a flang failure in the pre-merge testing for example.