Stream: project-inline-asm

Topic: LLVM feature/version check for asm


Josh Triplett (May 16 2020 at 18:55, on Zulip):

As mentioned on the GitHub PR for the implementation: Debian's Rust is built with system LLVM, Firefox is built with Rust, and Firefox uses "RUSTC_BOOTSTRAP". More generally, I think it'd be unfortunate if rustc built against system LLVM simply didn't support asm!, when nightly built against its own LLVM does. But it'd also be unfortunate if rustc built against system LLVM silently broke inline asm!.

Josh Triplett (May 16 2020 at 18:56, on Zulip):

@Amanieu Long-term, I agree with your comment in the thread, that you can just detect LLVM 11 (assuming all the patches make it into LLVM 11).

Josh Triplett (May 16 2020 at 18:56, on Zulip):

But in the meantime, I expect that your patches will make their way into distribution versions of LLVM 10.

Josh Triplett (May 16 2020 at 18:56, on Zulip):

(with some nudging)

Josh Triplett (May 16 2020 at 18:57, on Zulip):

You said that LLVM 10 will have all the patches needed for asm!, except for one needed for handling SSE registers?

Josh Triplett (May 16 2020 at 18:58, on Zulip):

(I expect that many people using asm! for systems programming won't care as much, but people using it to write vectorized code for performance will care.)

Amanieu (May 16 2020 at 18:58, on Zulip):

Yes.

Josh Triplett (May 16 2020 at 18:58, on Zulip):

Could you provide a link to that patch so I can take a look at it?

Josh Triplett (May 16 2020 at 18:58, on Zulip):

(Also, has it been merged for LLVM 11?)

Amanieu (May 16 2020 at 18:59, on Zulip):

It's really only 2 patches: https://github.com/rust-lang/llvm-project/pull/52

Amanieu (May 16 2020 at 18:59, on Zulip):

(yes)

Amanieu (May 16 2020 at 18:59, on Zulip):

1st one allows ${0:x} style modifiers to be used with intel syntax.

Amanieu (May 16 2020 at 19:00, on Zulip):

2nd one adds support for the x, t, g modifiers from gcc. These print a register as xmm, ymm or zmm respectively.

Josh Triplett (May 16 2020 at 19:00, on Zulip):

/me looks at what the system LLVM handling in rust-lang/rust looks like.

Josh Triplett (May 16 2020 at 19:00, on Zulip):

Looking at the comments in that LLVM PR: would it theoretically be possible for an LLVM point release to add a patch that adds the modifiers but not the ABI-breaking bits?

Josh Triplett (May 16 2020 at 19:01, on Zulip):

You said that'd be enough for inline asm.

Amanieu (May 16 2020 at 19:01, on Zulip):

The ABI-breaking bits are from the 1st patch, which is already in LLVM 10

Josh Triplett (May 16 2020 at 19:01, on Zulip):

Oh!

Josh Triplett (May 16 2020 at 19:01, on Zulip):

Missed that detail. So the changes you need on top of LLVM 10 are ABI-compatible?

Amanieu (May 16 2020 at 19:01, on Zulip):

And we really only needed a subset of that patch.

Josh Triplett (May 16 2020 at 19:01, on Zulip):

They're just, effectively, bugfixes?

Amanieu (May 16 2020 at 19:01, on Zulip):

Yes.

Josh Triplett (May 16 2020 at 19:02, on Zulip):

Then what are the chances we can get them into LLVM 10.0.1 at this point?

Amanieu (May 16 2020 at 19:02, on Zulip):

Honestly if it becomes an issue for firefox I expect debian to just backport that patch.

Josh Triplett (May 16 2020 at 19:03, on Zulip):

Well, yes, but the issue is whether we can detect that.

Josh Triplett (May 16 2020 at 19:03, on Zulip):

If we can get the patches into LLVM 10.0.1, and then have a version check that makes sure system LLVM is at least 10.0.1, that would make things much easier on people.

Amanieu (May 16 2020 at 19:05, on Zulip):

Also this is likely to be an issue: https://bugs.llvm.org/show_bug.cgi?id=45291

Josh Triplett (May 16 2020 at 19:06, on Zulip):

Ow. Yes, that seems likely to come up. Yikes.

Josh Triplett (May 16 2020 at 19:07, on Zulip):

I'm not especially familiar with LLVM development. Is it common that a bugfix with patch like that would sit in the bug tracker for two months with no response? Is there somewhere else it should be posted to get attention?

Josh Triplett (May 16 2020 at 19:09, on Zulip):

In any case, what do you think are the chances that you could poke upstream LLVM and get the patches you need into 10.0.1 before -rc1 gets cut?

Josh Triplett (May 16 2020 at 19:18, on Zulip):

At the moment, I'm wondering what our primary use case is for supporting excessively old versions of system LLVM, and whether we could get away with just hard-requiring 10.0.1 (or whatever has the patches). But if we can't do that, then it doesn't look too hard to just detect the version and if not found then disable asm!, or perhaps disable asm! support for SSE register in/out.

Josh Triplett (May 16 2020 at 19:18, on Zulip):

From what I can tell, it's easy enough in src/librustc_llvm/build.rs to detect the LLVM version and set a feature flag for rustc.

Amanieu (May 16 2020 at 19:20, on Zulip):

I don't want to disable all the asm tests on CI though (which uses system LLVM).

Amanieu (May 16 2020 at 19:20, on Zulip):

At the moment it can run all of the tests that don't reach codegen (i.e. syntax & type checking of asm!).

Josh Triplett (May 16 2020 at 19:23, on Zulip):

Once the new LLVM is released, couldn't we use that released version in CI?

Amanieu (May 16 2020 at 19:31, on Zulip):

CI uses the system LLVM from ubuntu 18.04, which is ancient

Amanieu (May 16 2020 at 19:32, on Zulip):

LLVM 6 it seems

Josh Triplett (May 16 2020 at 19:32, on Zulip):

Gah.

Josh Triplett (May 16 2020 at 19:32, on Zulip):

I wonder if we could fix that.

Josh Triplett (May 16 2020 at 19:32, on Zulip):

That would then make sure we're running all the tests that require newer system LLVM.

Josh Triplett (May 16 2020 at 19:33, on Zulip):

@Pietro Albini, do you happen to know what it would take to run CI with a newer system LLVM? (Or, do you know the right person to ask about that?)

Amanieu (May 16 2020 at 19:33, on Zulip):

That's only for pull request CI though. bors builds Rust's LLVM and uses that.

Pietro Albini (May 16 2020 at 19:34, on Zulip):

@Josh Triplett hmm, I'm not 100% sure

Pietro Albini (May 16 2020 at 19:34, on Zulip):

one of the reasons we use system llvm there is that we don't have to rebuild it every time

Pietro Albini (May 16 2020 at 19:35, on Zulip):

which would increase PR build times a ton, as we can't upload caches from PR builders for security reasons (PR builders don't have the keys required)

Josh Triplett (May 16 2020 at 19:36, on Zulip):

Sure, it makes sense that they'd use system LLVM for that reason.

Josh Triplett (May 16 2020 at 19:36, on Zulip):

I'm wondering what it would take to update the image that they use, to something newer.

Pietro Albini (May 16 2020 at 19:37, on Zulip):

just a change in the dockerfile and making sure it still works

Pietro Albini (May 16 2020 at 19:37, on Zulip):

@simulacrum do you recall any reason not to bump the llvm in PR builders to the latest one available in ubuntu?

Josh Triplett (May 16 2020 at 19:38, on Zulip):

Ubuntu 20.04 LTS has LLVM 10.

Josh Triplett (May 16 2020 at 19:42, on Zulip):

(One reason I can think of to not bump the version would be to test in CI that we still build with older LLVM. On the other hand, I'm wondering where our requirement for "minimum supported system LLVM version" comes from, and what the process would be to bump that forward. But that's a separate question, and not one that would have to be resolved to make this work.)

simulacrum (May 16 2020 at 19:51, on Zulip):

Yes, we use that to test compatibility with older LLVM

simulacrum (May 16 2020 at 19:52, on Zulip):

We just a month or so ago bumped to the current one - 8 I think - usually that's initiated by @cuviper. I'm not sure we have a strict policy. I would be uncomfortable bumping to latest though

Josh Triplett (May 16 2020 at 19:55, on Zulip):

Well, in that case, we'd need to detect 10.0.1 (or whatever version will support inline assembly properly) and set a feature flag (one which we also set with our own LLVM). We then have to figure out how much we can still do if we don't have that feature flag.

simulacrum (May 16 2020 at 19:55, on Zulip):

Do we expect to need the testing in PR CI? Otherwise just testing with normal bors builders seems fine to me - if the story is that you need to wait for llvm 11 for inline asm (or llvm 10 + patches?) imo that seems fine

simulacrum (May 16 2020 at 19:56, on Zulip):

If people aren't using rustup to install their toolchain and need assembly it's on them to get an appropriate llvm configured I think

Josh Triplett (May 16 2020 at 19:57, on Zulip):

simulacrum said:

If people aren't using rustup to install their toolchain and need assembly it's on them to get an appropriate llvm configured I think

I agree with this, but I don't want anyone experiencing subtle breakage. I'd want people to get an explicit error.

simulacrum (May 16 2020 at 19:57, on Zulip):

Definitely! I imagine we don't necessarily need version detection to do that though, or at least not exclusively

simulacrum (May 16 2020 at 19:58, on Zulip):

I'd be fine with a config.toml "yes my llvm is good enough" flag

Josh Triplett (May 16 2020 at 19:58, on Zulip):

Oh, that's a good idea. So, we could start with version detection, but we could also have a flag for "override the version detection, we've patched our system LLVM so asm works even though the version doesn't suggest it will"?

simulacrum (May 16 2020 at 19:58, on Zulip):

And we can or that with llvm version >= 11 or whatever is needed and eventually drop it once it becomes less necessary

simulacrum (May 16 2020 at 19:59, on Zulip):

Yep

simulacrum (May 16 2020 at 19:59, on Zulip):

Realistically I don't expect people to care that much

Josh Triplett (May 16 2020 at 20:00, on Zulip):

That seems reasonable to me. And if an LLVM point release comes out with all the patches we need, we can change the version requirement from LLVM 11 to that point release.

Josh Triplett (May 16 2020 at 20:00, on Zulip):

(Though we still need the patch from https://bugs.llvm.org/show_bug.cgi?id=45291 to get merged into LLVM 11.)

simulacrum (May 16 2020 at 20:00, on Zulip):

Sure, exact version doesn't matter to me

Josh Triplett (May 16 2020 at 20:00, on Zulip):

:+1:

Josh Triplett (May 16 2020 at 20:01, on Zulip):

For now, before that version of system LLVM is released, we could even just only have the "my system LLVM is good enough" flag.

Josh Triplett (May 16 2020 at 20:01, on Zulip):

And then we can add version detection to automatically set that once a version of LLVM comes out with all the patches we need.

Josh Triplett (May 16 2020 at 20:02, on Zulip):

@Amanieu Does that sound reasonable as a starting point, for now, to help reduce the likelihood that someone will use their system LLVM and run into bugs?

Josh Triplett (May 16 2020 at 20:02, on Zulip):

If so, I can write a comment summarizing that on the GItHub PR thread.

Josh Triplett (May 16 2020 at 20:14, on Zulip):

https://github.com/rust-lang/rust/pull/69171#issuecomment-629700314

Amanieu (May 16 2020 at 20:35, on Zulip):

My feeling is that anyone using nightly Rust should be using Rust's LLVM.

Amanieu (May 16 2020 at 20:35, on Zulip):

And that we should defer the issue of LLVM version until stabilization.

Amanieu (May 16 2020 at 20:36, on Zulip):

I think any bugs that people will encounter will be sufficiently obvious.

Josh Triplett (May 16 2020 at 20:37, on Zulip):

The SSE register thing might be (though people running into it might use it as an argument that the new asm! isn't insulating from LLVM bugs the way it was promised to).

Josh Triplett (May 16 2020 at 20:37, on Zulip):

The "forgets intel dialect" bug, though, seems really obscure.

Josh Triplett (May 16 2020 at 20:38, on Zulip):

Also, again, some people use nightly features on stable, which normally I'd write off as problematic except that Firefox does it.

Amanieu (May 16 2020 at 20:39, on Zulip):

Could we leave this for a follow-up PR though?

Josh Triplett (May 16 2020 at 20:39, on Zulip):

What is the downside of introducing a check here?

Josh Triplett (May 16 2020 at 20:39, on Zulip):

Depends. Would the follow-up PR happen before the next beta gets branched?

Josh Triplett (May 16 2020 at 20:40, on Zulip):

(Also, would implementation help make it more reasonable here?)

Amanieu (May 16 2020 at 20:40, on Zulip):

I expect the discussion on that to be much shorter.

Josh Triplett (May 16 2020 at 20:40, on Zulip):

I'd expect it to be unobjectionable, yeah.

Josh Triplett (May 16 2020 at 20:41, on Zulip):

I do understand wanting to get this PR done rather than making it interminable. :)

Josh Triplett (May 16 2020 at 20:41, on Zulip):

I'm happy to be the reviewer for the follow-up.

Josh Triplett (May 16 2020 at 20:42, on Zulip):

Can you note it in the tracking issue (with link to that comment), assuming you're willing to go with that plan?

Amanieu (May 16 2020 at 20:42, on Zulip):

Sure.

Josh Triplett (May 16 2020 at 20:42, on Zulip):

Links to the relevant LLVM patches would help as well, in the new tracking bullet point.

Josh Triplett (May 16 2020 at 20:42, on Zulip):

Thanks!

Last update: Jun 05 2020 at 22:25UTC