This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Consolidate llvm::CodeGenOpt::Level handling
ClosedPublic

Authored by scott.linder on Jan 17 2023, 2:51 PM.

Details

Summary

Add free functions llvm::CodeGenOpt::{getLevel,getID,parseLevel} to
provide common implementations for functionality that has been
duplicated in many places across the codebase.

Diff Detail

Event Timeline

scott.linder created this revision.Jan 17 2023, 2:51 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 17 2023, 2:51 PM
scott.linder requested review of this revision.Jan 17 2023, 2:51 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJan 17 2023, 2:51 PM

I wasn't sure whether to include getID and unit-test it, or just drop it until it is used. If anyone has a preference let me know and I'll update the review.

arsenm accepted this revision.Jan 17 2023, 2:59 PM

The error handling inconsistency is kind of annoying too but this is an improvement. I thought traditionally GCC accepted whatever big number you put after -O

This revision is now accepted and ready to land.Jan 17 2023, 2:59 PM
scott.linder added inline comments.Jan 17 2023, 3:02 PM
llvm/include/llvm/Support/CodeGen.h
57

This is ABI breaking, so maybe we don't want/need to define the underlying type?

arsenm added inline comments.Jan 17 2023, 3:06 PM
llvm/include/llvm/Support/CodeGen.h
57

This isn't in the C API, and this isn't for a release branch so I don't think it matters

barannikov88 added inline comments.
llvm/include/llvm/Support/CodeGen.h
57

Why did you need to restrict the underlying type?

72

Should return IDType.

llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
44

This disagrees with cl::init('2')

barannikov88 added inline comments.Jan 17 2023, 3:31 PM
llvm/include/llvm/Support/CodeGen.h
67

As I can see, clients do not check for nullopt. Either add checks or replace this check with an assertion and drop std::optional (in this case parseLevel should be updated accordingly).

scott.linder marked 2 inline comments as done.
  • Fix return type of getID
  • Fix mistakenly updated option help text
  • Add back in missing asserts
scott.linder added inline comments.Jan 18 2023, 4:00 PM
llvm/include/llvm/Support/CodeGen.h
57

There is no strict need, it just:

  • Avoids potential UB (casting an out-of-range value to an enumeration type without a fixed underlying type is undefined); in practice there is plenty of UB in LLVM, and a bug here wouldn't be a particularly pernicious kind of UB anyway.
  • Means users of the type might pack better (and we won't run out of 255 opt levels anytime soon).

I originally intended to change this to an enum class rather than a class in a namespace, but that is slightly more disruptive and should probably be done for every other enumeration defined here at the same time.

67

Good catch! I was working off of the old behavior of llvm::Optional and assuming the new std::optional was guaranteed to assert on dereference as well. I think the right interface is to expose the optional, so for the callsites which currently do the equivalent of asserting I will just carry over an explicit assert to the new version.

I posted to https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/26 to discuss the issue more generally, as at least some of the patches which moved code from llvm::Optional to std::optional accidentally dropped assertions.

barannikov88 added inline comments.Jan 18 2023, 4:24 PM
llvm/include/llvm/Support/CodeGen.h
57

Thanks for the explanation.

Means users of the type might pack better (and we won't run out of 255 opt levels anytime soon).

The downside is that int is usually more efficient.

Avoids potential UB [...]

cppreference says

Values of unscoped enumeration type are implicitly-convertible to integral types. If the underlying type is not fixed, the value is convertible to the first type from the following list able to hold their entire value range: int, unsigned int, ...

so casting to int should be safe.
Anyways, this is just a nit, feel free to ignore.

barannikov88 accepted this revision.Jan 18 2023, 4:25 PM
This revision was landed with ongoing or failed builds.Jan 23 2023, 2:51 PM
This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.
llvm/include/llvm/Support/CodeGen.h
67

This causes massive amounts of warnings when built with GCC:

../include/llvm/Support/CodeGen.h:67:12: warning: comparison is always false due to limited range of data type [-Wtype-limits]
   67 |     if (ID < 0 || ID > 3)
      |         ~~~^~~
scott.linder added inline comments.Jan 23 2023, 3:03 PM
llvm/include/llvm/Support/CodeGen.h
67

Fixed in 2dc33713de7ceae3e28a13be1d72c862ec0efb2c

Thank you for reporting, sorry for the noise!

I also ended up changing the underlying type to int to resolve https://lab.llvm.org/buildbot/#/builders/61/builds/38754 , and I removed the getID function (with the implicit conversions it would never get used anyway).

aaronpuchert added inline comments.
flang/lib/Frontend/FrontendActions.cpp
591

Does this need llvm::?

../llvm-project/flang/lib/Frontend/FrontendActions.cpp:590:7: error: use of undeclared identifier 'CodeGenOpt'; did you mean 'llvm::CodeGenOpt'?
      CodeGenOpt::getLevel(CGOpts.OptimizationLevel);
      ^~~~~~~~~~
      llvm::CodeGenOpt
../llvm-project/llvm/include/llvm/Support/CodeGen.h:53:13: note: 'llvm::CodeGenOpt' declared here
  namespace CodeGenOpt {
            ^

See https://lab.llvm.org/buildbot/#/builders/191/builds/13832.

scott.linder added inline comments.Jan 23 2023, 3:53 PM
flang/lib/Frontend/FrontendActions.cpp
591

Yes, I just commited a1baa7a5c57ea3065f8f354d2fc758b3328040fd to fix this; I thought I was building flang locally but didn't verify it and clearly was not

Thank you and sorry for the noise!