Page MenuHomePhabricator

systemz: allow configuring default CLANG_SYSTEMZ_ARCH
ClosedPublic

Authored by xnox on Mar 10 2020, 5:46 AM.

Details

Reviewers
uweigand
Group Reviewers
Restricted Project
Restricted Project
Commits
rG9c9d88d8b1bb: [SystemZ] Allow configuring default CLANG_SYSTEMZ_ARCH
Summary

On Ubuntu, we want to raise default CLANG_SYSTEMZ_ARCH to z13, thus allow configuring this via CMake.
On Debian, we want to raise it to z196.

Diff Detail

Event Timeline

xnox created this revision.Mar 10 2020, 5:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 5:46 AM
Herald added a subscriber: mgorny. · View Herald Transcript
xnox added reviewers: Restricted Project, Restricted Project.Mar 10 2020, 5:47 AM

Thanks for working on this! A few comments inline.

CMakeLists.txt
315

Should be moved above the set(CLANG_VENDOR, ... line.

Also, this new variable itself have a set line, so that we can provide a descriptive text, and also move the "z10" default to here instead of the sources.

Finally, I think the mixed-case name doesn't really match the common naming conventions here, what about CLANG_SYSTEMZ_DEFAULT_ARCH ?

xnox updated this revision to Diff 252554.Mar 25 2020, 6:13 AM
xnox edited the summary of this revision. (Show Details)
xnox retitled this revision from systemz: allow configuring default SystemZTargetCPU to systemz: allow configuring default CLANG_SYSTEMZ_ARCH.

Thanks for working on this! A few comments inline.

Updated! @uweigand Please re-review.

uweigand accepted this revision.Mar 25 2020, 6:31 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 25 2020, 6:31 AM

I cannot commit the patch myself, please "sponsor" committing this change.

This doesn't apply cleanly to current mainline. Can you rebase and test again? I'll check it in then.

xnox updated this revision to Diff 253251.Mar 27 2020, 4:50 PM
xnox marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
Hahnfeld added inline comments.
clang/CMakeLists.txt
311 ↗(On Diff #253559)

Passing values like this is unusual for CMake and causes all source files to be recompiled if the value is changed. Instead could we add this to include/clang/Config/config.h.cmake like other CLANG_DEFAULT_* options?

Ah, good point. Dimitry, can you prepare an updated patch to implement Jonas' suggestion?

thakis added inline comments.
clang/CMakeLists.txt
311 ↗(On Diff #253559)

+1

thakis added inline comments.Mar 30 2020, 11:03 AM
clang/CMakeLists.txt
311 ↗(On Diff #253559)

Also, this is used in one cpp file; passing the define to every compilation unit seems a bit aggressive.

I'll try to land a patch to fix this.

thakis added inline comments.Mar 30 2020, 11:17 AM
clang/CMakeLists.txt
311 ↗(On Diff #253559)

Done in c506adcdf2ca3ba6459e52e09c55868e3b57af46. (This only changes the implementation of the feature; the way people who build clang tell cmake what they want didn't change.)

Sorry for being too slow =) I like all the fixes done to move the define into a more natural config.h location, thank you.

Thanks for working on this, @thakis !