Page MenuHomePhabricator

mlir: replace llvm::None with std::nullopt (NFC)
AbandonedPublic

Authored by artagnon on Nov 29 2022, 11:57 PM.

Details

Summary

This is part of an effort to migrate from llvm::Optional to
std::optional.

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:57 PM
artagnon requested review of this revision.Nov 29 2022, 11:57 PM
artagnon updated this revision to Diff 478847.Nov 30 2022, 1:25 AM

Use using llvm::None to fix compile-time errors.

kazu added a comment.Nov 30 2022, 9:45 AM

Use using llvm::None to fix compile-time errors.

Are you trying to replace llvm::None first in this patch and then address None (without llvm::) later?

mlir/include/mlir/Support/LLVM.h
139 ↗(On Diff #478847)

Do we need this? We try our best to use std:: explicitly:

https://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std

artagnon updated this revision to Diff 479000.Nov 30 2022, 9:51 AM

Address review comment by @kazu, git clang-format.

artagnon marked an inline comment as done.Nov 30 2022, 9:53 AM

Unfortunately, there is no easy solution to replacing None without the llvm:: prefix. I edited it by hand in a few places.

mlir/include/mlir/Support/LLVM.h
139 ↗(On Diff #478847)

Thanks for catching this! It seems to be a stray edit.

artagnon marked an inline comment as done.Nov 30 2022, 10:21 AM

Use using llvm::None to fix compile-time errors.

Are you trying to replace llvm::None first in this patch and then address None (without llvm::) later?

Yes, I know what happened: I did a replacement of llvm::None with std::nullopt in mlir/, and replaced the instance in using llvm::None by mistake. Then, I was puzzled about the compile failure: that was the reason for the stray edit. Anyway, the diff should be good now.

kazu added a comment.Nov 30 2022, 10:27 AM

Unfortunately, there is no easy solution to replacing None without the llvm:: prefix. I edited it by hand in a few places.

Have you considered putting [[deprecated]] in front of inline constexpr std::nullopt_t None = std::nullopt; in llvm/include/llvm/ADT/None.h? Doing so will cause the compiler to warn you every time you use None or llvm::None. You can then extract warnings from your build log and use sed and such to automatically replace llvm::None and None with std::nullopt. If I were you, I would do this mechanical replacements in non-generated code first because if you touch tablegen or .td files, you may have to update corresponding tests that check the automatic generation. You might even choose to update comments in a separate patch. The idea is to break up the big migration into series of super safe steps that are easy to review.

artagnon updated this revision to Diff 479018.Nov 30 2022, 10:40 AM

Address review comment by @kazu. Now, the patch is generated by the
following steps:

$ find . -exec gsed -i s/llvm::Optional/std::nullopt/ {} \;
$ git checkout -- include/MLIR/Support/LLVM.h # stray edit
$ git commit
$ git clang-format @~
$ git commit --amend

Thanks for the suggestion @kazu! I'll try to tackle that in a follow-up patch. For now, I've updated the diff to a programmatically-generated and safe-subset replacement of llvm::Optional to std::nullopt. We should have additional assurance from the CI. I understand your concerns about wanting super-safe steps, but D138934 involved a lot of manual work, and I'm not sure how we can fix that: will it be sufficient to trust the CI in that case?

kazu added a comment.Nov 30 2022, 11:03 AM

Thanks for the suggestion @kazu! I'll try to tackle that in a follow-up patch. For now, I've updated the diff to a programmatically-generated and safe-subset replacement of llvm::Optional to std::nullopt. We should have additional assurance from the CI. I understand your concerns about wanting super-safe steps, but D138934 involved a lot of manual work, and I'm not sure how we can fix that: will it be sufficient to trust the CI in that case?

I am not sure how much we can rely on CI. For example, we may not be able to catch performance regressions of generated code. llvm/include/llvm/Support/Casting.h knows about Optional but not std::optional yet, so dyn_cast of an std::optional object quietly returns nullptr, which could in turn cause a missed optimization. There may be other pitfalls like this, so I am not sure if there is a better way than go through every single instance one by one.

mlir/include/mlir/Dialect/Affine/Analysis/AffineAnalysis.h
146

Did you mean to replace this one also?

mlir/unittests/Transforms/DialectConversion.cpp
18

Did you mean to replace these also?

artagnon updated this revision to Diff 479027.Nov 30 2022, 11:10 AM

Missed /g in gsed's replace command

Thanks for the suggestion @kazu! I'll try to tackle that in a follow-up patch. For now, I've updated the diff to a programmatically-generated and safe-subset replacement of llvm::Optional to std::nullopt. We should have additional assurance from the CI. I understand your concerns about wanting super-safe steps, but D138934 involved a lot of manual work, and I'm not sure how we can fix that: will it be sufficient to trust the CI in that case?

I am not sure how much we can rely on CI. For example, we may not be able to catch performance regressions of generated code. llvm/include/llvm/Support/Casting.h knows about Optional but not std::optional yet, so dyn_cast of an std::optional object quietly returns nullptr, which could in turn cause a missed optimization. There may be other pitfalls like this, so I am not sure if there is a better way than go through every single instance one by one.

Interesting. Okay, let's try to go through the diff by hand, and find any issues: thanks a lot for volunteering to review this; it's definitely not easy.

artagnon abandoned this revision.Dec 13 2022, 4:27 AM

Subsumed by 1a36588ec64 by @kazu.