Refine instruction_set MIR inline rules

[Lokathor]

Previously an exact match of the instruction_set attribute was required for an MIR inline to be considered. This change checks for an exact match only if the callee sets an instruction_set in the first place. When the callee does not declare an instruction set then it is considered to be platform agnostic code and it's allowed to be inline'd into the caller.

cc @oli-obk

[Edit] Zulip Context: https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/What.20exactly.20does.20the.20MIR.20optimizer.20do.3F

[rustbot]

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[Lokathor]

The tests failed because of checking the exact output of the MIR inliner, but I don't know enough about how the test system works to know what I should be changing there.

https://github.com/Lokathor/rust/blob/mir-opt-when-instruction-set-missing-on-callee/src/test/mir-opt/inline/inline_instruction_set.rs

[Kobzol]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 0657585894a4f443d39ecd23fe5f5c1998bd9edf with merge ae9bb307e7ad590270685e7f486cbfba1ca5a7d9...

[cjgillot]

@Lokathor you can run ./x.py test --bless mir-opt to update the test outputs.

[Lokathor]

@Lokathor you can run ./x.py test --bless mir-opt to update the test outputs.

was really hoping i wouldn't have to clone the repo :(

[Lokathor]

Alright I'm trying to update the test to match the new compatibility definition, and I'm not even clear what the test is trying to do in the first place.

Should each type of test function just try to call each other type of function, and then we're just going to bless whatever the MIR that comes out is? Or like... what's being asserted?

[Lokathor]

On advice of the dark-arts channel I just updated the test comments and blessed the new output without changing the test code at all.

[tmiasko]

I nominated for language team to confirm that:

When the callee does not declare an instruction set then it is considered to be platform agnostic code and it's allowed to be inline'd into the caller.

(One consequence of that choice is that functions with inline assembly cannot rely on the target default being used, so they should specify the instruction set)

[Lokathor]

Just based on the comments in the file I touched, it looks possible to restrict the inline based on the content of the callee's body after the attribute check passes, though I personally don't know how to actually check for inline asm in the body and pass/fail based on that. Someone else might have to do that part.

I'll say that it's possible to write asm that's safe to mix between both instruction sets, and I've actually got code like that in the gba crate already. However, writing such code is extra tricky because in many situations the allowed options will differ between the a32 and t32 version (t32 will allows preserves_flags far less often).

EDIT: actually, on CPUs that do interworking, inline asm in a library function is kinda dangerous already unless steps are taken to offset the danger. If code is written assuming an arm target and it sets options(preserves_flags), then it's entirely possible for that library to also compile without incident on a thumb target, but when compiled as thumb code the flags wouldn't be preserved. Now there's silent UB in the library. Since a library can't control what target it's built for, any such library should really be using cfg to prevent this problem.

[JakobDegen]

I think an alternative interpretation is that there cannot be "negative" instruction set annotations (ie ones that shrink the set of permitted instructions), only "positive" ones

[Lokathor]

I don't quite understand what you're saying there.

t32 has less possible instructions than a32. Actually it is more precise to say it has less possible mnemonics, because those mnemonics are encoded totally differently. So in terms of the actual physical binary encoding there is a 0% overlap between the two.

EDIT: also, the thumb-mode CPU feature in LLVM is what reduces what you can encode, so this doesn't work like how having avx enabled on a function allows you to do "all the pre-avx things, and also avx things too".

[fee1-dead]

Rerolling, as I don't know about instruction_set in MIR.

r? compiler

[JakobDegen]

Oh, I had misunderstood this. I am somewhat surprised by the suggested semantics tbh, I would've expected no instruction set to mean the target default. Guess we'll see what T-lang has to say

[Lokathor]

That's the current semantics, but that makes performance tank and makes basically all code in other crates unusable, an example of this is shown in the zulip thread.

[JakobDegen]

Under the proposed semantics, is it possible to write a correct inline asm block in a function that does not have an instruction set attribute?

[Lokathor]

It is possible. The main difference is that with t32 you can't use preserves_flags as often (there are other differences as well).

I'm going to preemptively guess what you're gonna say next and say that we should inspect the function body of no-attribute functions and block their MIR inline if an asm block is detected in the body.

But I don't know to do that specifically. So if someone could tell me, or just put in a PR to my branch that makes that happen, it would be appreciated.

EDIT: for an example of inline asm that's actually specifically intended to work perfectly fine in both instruction sets: https://docs.rs/gba/0.10.0/src/gba/gba_cell.rs.html#97-133, and in this example it actually must be inline asm that's used rather than any intrinsic because LLVM's current handling of atomics on old embedded platforms is "very poor".

[tmiasko]

Could you also squash the commits?

[Lokathor]

I could squash them if you tell me exactly what to type into the terminal to make that happen. Every time I've ever tried to do anything with git other than commit and push i break it.

[oli-obk]

I could squash them if you tell me exactly what to type into the terminal to make that happen. Every time I've ever tried to do anything with git other than commit and push i break it.

git fetch upstream # or whatever you named your remote that points to rust-lang/rust, not your own fork
git rebase -i upstream/master # opens a text editor, replace all `pick` with `fixup` if you want to squash them into the commit above them.
git push --force-with-lease

[Lokathor]

I did that and a bunch of stuff pulled and tidy ran again and github didn't change. So someone tried to help me solve it and there wasn't a remote origin set anymore, so I set that and now I'm on the wrong branch?

daniel@ramiel:~$ git remote show
daniel@ramiel:~$ git remote add origin git@github.com:Lokathor/rust.git
daniel@ramiel:~$ git remote show
origin
daniel@ramiel:~$ git push --force-with-lease
fatal: The current branch master has no upstream branch.
To push the current branch and set the remote as upstream, use
    git push --set-upstream origin master

So I'm not sure if I should do the thing it's suggesting I do, but it sounds dangerous so I figured I'd ask first.

I'm perhaps the worst at git out of all Rust contributors.

[oli-obk]

That command is harmless and correct, but you should check that your commits look alright first I guess

[Lokathor]

daniel@ramiel:~/rust$ git status
On branch mir-opt-when-instruction-set-missing-on-callee
Your branch and 'origin/mir-opt-when-instruction-set-missing-on-callee' have diverged,
and have 1852 and 7 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)
nothing to commit, working tree clean

This... doesn't look right.

[tmiasko]

There have been 1851 new commits since your last push, so that seems exactly right if you squashed all commits into one.

[Lokathor]

I did that push command and git said everything worked but the PR isn't showing any change.

[thomcc]

Don't we have bors squash? https://bors.rust-lang.org/ indicates we do, or is it not a command that actually works.

[tmiasko]

@bors r+

[bors]

📌 Commit ea47943 has been approved by tmiasko

It is now in the queue for this repository.