search

ykjit / yk

yk packages
https://ykjit.github.io/yk/
Other
28 stars 7 forks source link

Partially implement fp_to_si. #1283

Closed vext01 closed 2 weeks ago

vext01 commented 2 weeks ago

This allows the lua test suite to run to completion (with serialised compilation).

Requires https://github.com/ykjit/ykllvm/pull/179

ltratt commented 2 weeks ago

with serialised compilation

Just to check: it also works with serialised compilation off, right? [I appreciate that serialised compilation in some senses tests more of the pipeline, but I wanted to be sure.]

vext01 commented 2 weeks ago

I hadn't tested it previously, but yes it does (tested with debug build only).

(At this stage we are probably exiting before traces get a chance to compile?)

vext01 commented 2 weeks ago

(At this stage we are probably exiting before traces get a chance to compile?)

Yep. Just checked. No traces are executed if you don't serialise compilation.

ltratt commented 2 weeks ago

Got it.

Can we add yklua (the current HEAD commit on main) to .buildbot.sh as part of this PR?

vext01 commented 2 weeks ago

Mind if we do it in the next PR, so that others can run a complete test suite in the meantime?

ltratt commented 2 weeks ago

I don't mind doing it in the next PR but I don't know what "others can run a complete test suite in the meantime" means.

vext01 commented 2 weeks ago

Just means the test suite can be run to completion while I prepare the change.

ltratt commented 2 weeks ago

Agreed offline about not adding yklua tests into this PR.

@vext01 When the ykllvm PR merges can you force push an update to the ykllvm merge commit in this PR? It will save us well over an hour of CI time.

vext01 commented 2 weeks ago

Ah, because we cache the previous commit? That's unfortunate.

ltratt commented 2 weeks ago

ykllvm caching will do so on the merge commit. So when this PR merges there won't be a cache hit, so this PR takes ~45-50 mins to merge, then we realise that the ykllvm we care about wasn't cached, and do so then.

Everything works in either case, but I think we'll have several yk PRs merging today, so it's probably good not to tie CI up for so long.

vext01 commented 2 weeks ago

OK, so in the future I won't prepare the PR with an LLVM sync from the outset.

ltratt commented 2 weeks ago

I don't want to make our lives too complex here. In fact, in retrospect, I think I should have kept my mouth shut! Let's just let CI do its thing as-is without force pushing. After lunch it'll catch up this.