Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of ykconfig -ao. #1411

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Get rid of ykconfig -ao. #1411

merged 1 commit into from
Oct 3, 2024

Conversation

vext01
Copy link
Contributor

@vext01 vext01 commented Oct 3, 2024

Requires: ykjit/ykllvm#209 (so will need a push to re-sync ykllvm)

Not so long ago we added yk-config -ao <level> to allow us to run our pre-canned franken-O1 pipeline for the entire test suite. This was only ever designed as an interim hack, and we've wanted Yk to simply work with standard optimisation flags like -O1, -O2 etc.

This change is a step in that direction.

We remove -ao and our non-standard defaults and add the ability for individual tests to specify extra compiler flags. This gives us the ability to specify optimisation parameters on a per-test basis.

I then ported as many tests as were practical to use -O1. The remaining tests (that were either too involved for me to convert right now, or actually rely on an IR shape generated by our old default pipeline) I've left using our franken-O1 by passing:

-O0 -Xclang -disable-O0-optnone -Xlinker
--lto-newpm-passes=instcombine<max-iterations=1;no-use-loop-info;no-verify-fixpoint>

We do have to disable yklua in CI for now, as yklua still uses yk-config -ao and we have to break a circular dependency.

bin/yk-config Outdated
debug*) OUTPUT="$OUTPUT -g" ;;
esac

# Disable backend optimisations that Yk can't handle by adding the
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed a mistake here. We have a linker flag in the --cflags section!

Will fix and retest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d874f9c

@@ -1,4 +1,5 @@
// ignore-if: test $YK_CARGO_PROFILE != "debug"
// Compiler:
// env-var: YKT_EXTRA_CC_FLAGS=-O1 -g
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does YKT mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be YKB_EXTRA_...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YKT was traditionally used for testing only, but I see it's been renamed/removed at some point.

You can't use this variable outside of tests since it requires running the compiler via the compiler wrapper. Is that OK still?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just rename it to YKB since it's a build-time option (even if only "build testing")? If you think that's the way to go, you can rename + squash in one go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

I must admit I get confused at times with our variables. I've seen so many reincarnations of them that I'm just constantly lost!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the feeling! FWIW, I think the prefixes are now documented in the book tolerable well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah mostly. There's a couple of things that often still catch me out:

  • variables with no D or B, like YK_LOG. I always type YKD_LOG.
  • the mandatory <path>: prefix of YKD_LOG_IR, because I was spoiled by the optional prefix of YK_LOG :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the mandatory : prefix of YKD_LOG_IR, because I was spoiled by the optional prefix of YK_LOG :)

Hmm, you're right, that shouldn't be mandatory! PR welcome!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. It's just such a small fish right now!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway YKT -> YKB in a04c8f8

@ltratt
Copy link
Contributor

ltratt commented Oct 3, 2024

Other than YKT/YKB this looks good!

@ltratt
Copy link
Contributor

ltratt commented Oct 3, 2024

Please squash.

@vext01
Copy link
Contributor Author

vext01 commented Oct 3, 2024

I've also sneaked in 3eda80d which will be needed for the yklua tests when we re-enable them.

Its a flag we already have enabled for link time, but is also needed for pre-link time, since that pas can (and does) run at both times.

@ltratt
Copy link
Contributor

ltratt commented Oct 3, 2024

Yes I saw it. Please squash.

@vext01
Copy link
Contributor Author

vext01 commented Oct 3, 2024

squashed.

@ltratt ltratt added this pull request to the merge queue Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 3, 2024
@vext01
Copy link
Contributor Author

vext01 commented Oct 3, 2024

My apologies. I force pushed a change to sync in ykllvm too. Ready.

@ltratt ltratt added this pull request to the merge queue Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 3, 2024
@vext01
Copy link
Contributor Author

vext01 commented Oct 3, 2024

Just a test failing due to SWT recorder calls.

Fixed in 7187176

OK to squash?

@ltratt
Copy link
Contributor

ltratt commented Oct 3, 2024

Please squash.

Not so long ago we added `yk-config -ao <level>` to allow us to run our
pre-canned franken-O1 pipeline for the entire test suite. This was only
ever designed as an interim hack, and we've wanted Yk to simply work
with standard optimisation flags like -O1, -O2 etc.

This change is a step in that direction.

We remove `-ao` and our non-standard defaults and add the ability for
individual tests to specify extra compiler flags. This gives us the
ability to specify optimisation parameters on a per-test basis.

I then ported as many tests as were practical to use -O1. The remaining
tests (that were either too involved for me to convert right now, or
actually rely on an IR shape generated by our old default pipeline) I've
left using our franken-O1 by passing:

-O0 -Xclang -disable-O0-optnone -Xlinker \
    --lto-newpm-passes=instcombine<max-iterations=1;no-use-loop-info;no-verify-fixpoint>

We do have to disable yklua in CI for now, as yklua still uses
`yk-config -ao` and we have to break a circular dependency.
@vext01
Copy link
Contributor Author

vext01 commented Oct 3, 2024

squashed

@ltratt ltratt added this pull request to the merge queue Oct 3, 2024
Merged via the queue into ykjit:master with commit 567efc2 Oct 3, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants