Page MenuHomePhabricator

mlir/tblgen: use std::optional in generation
ClosedPublic

Authored by artagnon on Nov 29 2022, 11:11 AM.

Details

Summary

This is part of an effort to migrate from llvm::Optional to
std::optional. This patch changes the way mlir-tblgen generates .inc
files, and modifies tests and documentation appropriately. It is a "no
compromises" patch, and doesn't leave the user with an unpleasant mix of
llvm::Optional and std::optional.

A non-trivial change has been made to ControlFlowInterfaces to split one
constructor into two, relating to a build failure on Windows.

See also: https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

Signed-off-by: Ramkumar Ramachandra <r@artagnon.com>

Diff Detail

Event Timeline

artagnon created this revision.Nov 29 2022, 11:11 AM
artagnon requested review of this revision.Nov 29 2022, 11:11 AM
kazu added a comment.Nov 29 2022, 12:01 PM

@artagnon, thanks for your help. I have a couple of concerns:

  • You seem to touch formatting in a few places. Could you keep unrelated parts intact? Alternatively, you can format the entire file in a separate patch so that your modification will stand out by itself. You can use git clang-format HEAD~ to format the parts you touch.
  • Somewhat related to formatting, your patch touches the documentation a lot. If it's not related to the std::optional migration, please upload a separate patch.
  • llvm::None is interchangeable with std::nullopt. If I were you, I might consider replacing llvm::None with std::nullopt first. This way, even if this patch causes a regression and get reverted, the patch to migrate to std::nullopt should stay, resulting in a net progress.
artagnon updated this revision to Diff 478828.Nov 30 2022, 12:09 AM

Rebased on top of D138984.

artagnon updated this revision to Diff 479002.Nov 30 2022, 9:58 AM

Address some build failures in flang. Unfortunately, it's very
difficult to build flang locally, as it takes a really long time.
I'm relying on the CI to tell us about additional failures.

Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 9:58 AM
artagnon updated this revision to Diff 479191.Dec 1 2022, 1:24 AM

Fix all flang compile errors.

jpienaar added inline comments.Dec 5 2022, 3:21 PM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1342

Why couldn't this be std::optional<int64_t> ?

The Windows build is failing, and I don't know why. In light of @kazu's comments on D138984, and his recent commits, I'm tempted to abandon this change, and let him take over the work.

mlir/lib/Dialect/Shape/IR/Shape.cpp
1342

It could very well be. I suppose it's a matter of style: personally, I prefer auto to make the code less brittle to changes.

clementval added inline comments.
mlir/lib/Dialect/Shape/IR/Shape.cpp
1342
artagnon updated this revision to Diff 482453.Dec 13 2022, 6:40 AM

Rebase onto main, address review comments, and attempt to fix build on
Windows.

artagnon marked 2 inline comments as done.Dec 13 2022, 6:41 AM
artagnon added inline comments.
mlir/lib/Dialect/Shape/IR/Shape.cpp
1342

Fixed now.

artagnon edited the summary of this revision. (Show Details)Dec 14 2022, 3:24 AM
artagnon updated this revision to Diff 482787.Dec 14 2022, 3:26 AM
artagnon marked an inline comment as done.

Attempt to fix failing builds on Debian/Windows.

LGTM for the sparse part, since changes are mechanical in nature

jpienaar accepted this revision.Dec 16 2022, 1:24 PM

This looks direct enough and replaces deprecated struct, so LGTM.

This revision is now accepted and ready to land.Dec 16 2022, 1:24 PM

(@kazu seems to be doing related cleanups, so I'd leave last check to him)

artagnon updated this revision to Diff 483734.Dec 17 2022, 12:51 AM

Rebased onto main, with changes. Posting here for pre-merge CI checks.

This revision was automatically updated to reflect the committed changes.