Page MenuHomePhabricator

[flang] Upstream recent work on FIR to llvm-project.
ClosedPublic

Authored by schweitz on Apr 24 2020, 3:25 PM.

Diff Detail

Event Timeline

schweitz created this revision.Apr 24 2020, 3:25 PM
Herald added a project: Restricted Project. · View Herald Transcript
schweitz added a project: Restricted Project.

Would it be possible to update the commit message/summary to describe what's changed here?

A history of reviews and comments can be found here. https://github.com/flang-compiler/f18-llvm-project/pulls

rriddle added inline comments.
flang/lib/Optimizer/Dialect/FIROps.cpp
593

return success(llvm::is_contained(validNames, linkage)); ?

617

bodyRegion->front().addArguments(iterArgs.getTypes());?

802

Same here.

991

This could be written declaratively like:

let assemblyFormat = "($results^ `:` type($results))? attr-dict";

using optional groups:
https://mlir.llvm.org/docs/OpDefinitions/#optional-groups

1011

This seems weird. attr.getNumElements()?

1440

This doesn't seem to do anything? Seems WhereOp is already marked as RecursiveSideEffects which would cause it be deleted in cases where the bodies were empty.

schweitz updated this revision to Diff 260493.Mon, Apr 27, 4:35 PM

Include River's suggestions.

schweitz marked 5 inline comments as done.Mon, Apr 27, 4:40 PM
schweitz added inline comments.
flang/lib/Optimizer/Dialect/FIROps.cpp
617

This one caused a test to fail. Have to debug it.

1440

Removed. It doesn't look like empty WhereOps are removed either way.

schweitz marked an inline comment as done.Mon, Apr 27, 4:42 PM
schweitz updated this revision to Diff 260504.Mon, Apr 27, 5:23 PM

react to other review comments

schweitz marked 4 inline comments as done.Mon, Apr 27, 5:26 PM
schweitz added inline comments.
flang/lib/Optimizer/Dialect/FIROps.cpp
617

I don't get it. Deferring for now.

1440

Fixed the bug here.

schweitz added a reviewer: ftynse.
sscalpone accepted this revision.Mon, Apr 27, 5:44 PM
This revision is now accepted and ready to land.Mon, Apr 27, 5:44 PM
This revision was automatically updated to reflect the committed changes.

Would it be possible to update the commit message/summary to describe what's changed here?

I think it'd be great to keep the commit messages in the repo actually being descriptive: "Upstream recent work on FIR to llvm-project" does not tell us anything about the commit, compared to for example something like "Add MemoryEffects to FIR operations".

It also seems like this is mixing a bunch of different thing that really should be different commits instead (That may contributes to the difficulty to provide a clear commit message).

Would it be possible to update the commit message/summary to describe what's changed here?

I think it'd be great to keep the commit messages in the repo actually being descriptive: "Upstream recent work on FIR to llvm-project" does not tell us anything about the commit, compared to for example something like "Add MemoryEffects to FIR operations".

It also seems like this is mixing a bunch of different thing that really should be different commits instead (That may contributes to the difficulty to provide a clear commit message).

I completely agree with Mehdi here; a link to an external repo that has a proper commit history isn't a substitute for a meaningful commit message in our repo. A commit message like "Upstream recent work on FIR to llvm-project" tells me nothing about what the commit does if I end up needing to look back through the history/bisect a bug etc. It's also not sufficient to say "this is a set of commits that has been reviewed externally and that I'm upstreaming as one" because the community on the linked repository is different to (and much smaller than) the community of people that participate in Flang in LLVM.

It is usually polite to wait for someone who has requested changes to approve before committing; this doesn't seem to have happened with @rriddle's comments here. Especially since one of your replies to their comments was that you were unsure of something I think it would have been a lot better to give them time to clarify.
I also would have expected to be given time to reply on my request before this was committed, as I had requested something specific and would expect to at least be able to rebut a reply dismissing my concerns.