This is an archive of the discontinued LLVM Phabricator instance.

Use VersionTuple for parsing versions in Triple, fixing issues that caused the original change to be reverted. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible.
ClosedPublic

Authored by jamesfarrell on Dec 1 2021, 8:57 AM.

Diff Detail

Event Timeline

jamesfarrell created this revision.Dec 1 2021, 8:57 AM
jamesfarrell requested review of this revision.Dec 1 2021, 8:57 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 1 2021, 8:57 AM
jamesfarrell added inline comments.Dec 1 2021, 9:02 AM
llvm/lib/MC/MCStreamer.cpp
1343

This was broken in the original change, because we were blindly dereferencing an Optional without checking whether it had a value.

llvm/unittests/Support/Host.cpp
411

This was broken in the original change. Because it's wrapped in "#if defined(APPLE)", I'm not sure how to test it prior to commit.

danalbert accepted this revision.Dec 1 2021, 5:33 PM
This revision is now accepted and ready to land.Dec 1 2021, 5:33 PM

Don't use the preprocessor to hide test cases, so we can at least tell if it compiles when testing on platforms other than Apple and AIX.

danalbert accepted this revision.Dec 2 2021, 1:08 PM
thakis added a subscriber: thakis.Dec 6 2021, 9:58 AM

Either this or D115139 broke check-llvm on arm macs: http://45.33.8.238/macm1/23120/step_11.txt

getMacOSHostVersion sounds kind of triple-related, so I'm guessing it's this one.

Please take a look, and revert for now if it takes a while to fix.

thakis added a comment.Dec 6 2021, 9:59 AM

Oh, it's already reverted, apologies. We'll know if that fixed that bot in 15 min or so then. Something to keep in mind for relanding though :)

Oh, it's already reverted, apologies. We'll know if that fixed that bot in 15 min or so then. Something to keep in mind for relanding though :)

Yeah, I'm currently trying to build and test on my Mac laptop. It's not ARM tho, so if the test passes there I'm not sure what my next steps are to land this change.

thakis added a comment.Dec 7 2021, 5:22 AM

Oh, it's already reverted, apologies. We'll know if that fixed that bot in 15 min or so then. Something to keep in mind for relanding though :)

Yeah, I'm currently trying to build and test on my Mac laptop. It's not ARM tho, so if the test passes there I'm not sure what my next steps are to land this change.

It seems to also fail on other arm bots. Chances are it'll fail locally on intel machines if you run cmake with -DLLVM_DEFAULT_TARGET_TRIPLE=arm64-apple-darwin (or some other arm triple). Does that help? (I haven't tried it in this case, but most of the time that's sufficient to repro arm-host issues on non-host machines.)