Page MenuHomePhabricator

[mlir][CRunnerUtils] Enable compilation with C++11 toolchain on microcontroller platforms.
ClosedPublic

Authored by nicolasvasilache on Mar 11 2020, 3:05 PM.

Details

Summary

The C runner utils API was still not vanilla enough for certain use
cases on embedded ARM SDKs, this enables such cases.

Adding people more widely for historical Windows related build issues.

Diff Detail

Event Timeline

Update commit message.

aartbik accepted this revision.Mar 11 2020, 3:23 PM

LGTM if others see no Windows issue

mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
48

perhaps use () on the part after : too for readability
(assuming this does not violate some llvm brevity rule I don't know yet :-)

This revision is now accepted and ready to land.Mar 11 2020, 3:23 PM

What kind of constraints are we placing on the runner utils? The rest of the codebase is C++14, and it is quite likely that C++14 features might sneak in here later.

@rriddle

CRunnerUtils:
Entities in this file are must be retargetable, including on targets without a C++ runtime.
I'll add the must compile with C++11 constraint in doc too.

If you have suggestions on how to test this concretely, I'd be happy to add it.

RunnerUtils in general: no constraints, it includes CRunnerUtils.

flaub added a comment.Mar 11 2020, 4:02 PM

I don't have MSVC at this minute but this seems OK. Is it that the older compiler doesn't understand constexpr functions? I guess there's no fundamental reason the accessor operators need to be constexpr.

mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
48

Chaining multiple ternary operations just seems less readable than the original.

Adding Nate who is going to own the Windows MLIR Buildbot during my maternity :).

mehdi_amini added a comment.EditedMar 11 2020, 4:15 PM

@rriddle

CRunnerUtils:
Entities in this file are must be retargetable, including on targets without a C++ runtime.
I'll add the must compile with C++11 constraint in doc too.

If you have suggestions on how to test this concretely, I'd be happy to add it.

Force-build it in c++11 mode even when the rest of the project is built with c++14 or above?

The mlir-check target passes with MS C++ 19.24.28319 (except for the known failure). If there is another test that would be helpful on Windows let me know.

nicolasvasilache marked an inline comment as done.Mar 12 2020, 4:36 AM
nicolasvasilache added inline comments.
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
48

I agree but I have recollections from olden days that strict C++11 can only deduce the type if the body is a single line return statement.
The person who submitted this internally has a strict C++11 toolchain.
I found a few places that mention this limit (e.g. grep "single line" in https://en.cppreference.com/w/cpp/language/constexpr)
But I didn't go and dig into the standard for a 1-line readability regression.

nicolasvasilache marked an inline comment as done.Mar 12 2020, 4:36 AM
nicolasvasilache marked 2 inline comments as done.Mar 12 2020, 7:17 AM
nicolasvasilache added inline comments.
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
48

@flaub

I can confirm that with the CMake change to force C++11 I get the following error on the old code:

/usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/ExecutionEngine/CRunnerUtils.h:48:3: error: use of this statement in a constexpr function is a C++14 extension [-Werror,-Wc++14-extensions]
  if (N <= 1) return 1;
  ^
/usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/ExecutionEngine/CRunnerUtils.h:49:3: error: multiple return statements in constexpr function is a C++14 extension [-Werror,-Wc++14-extensions]
  return (isPowerOf2(N) ? N : 2 * nextPowerOf2((N + 1) / 2));
  ^
/usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/ExecutionEngine/CRunnerUtils.h:48:15: note: previous return statement is here
  if (N <= 1) return 1;

Force C++11 in cmake, address readability comment.

@stella.stamenova thank you and congrats ! :)

@NathanielMcVicar thanks for checking, I made a minor CMake addition, I do not expect it to break the Win build but please double check for me? Thanks!

This revision was automatically updated to reflect the committed changes.