-
Notifications
You must be signed in to change notification settings - Fork 103
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
Penultimate wasm SIMD update (hopefully) #101
Penultimate wasm SIMD update (hopefully) #101
Conversation
When lowering a BUILD_VECTOR SDNode, we choose among various possible vector creation instructions in an attempt to minimize the total number of instructions used. We previously considered using swizzles, consts, and splats, and this patch adds shuffles as well. A common pattern that now lowers to shuffles is when two 64-bit vectors are concatenated. Previously, concatenations generally lowered to sequences of extract_lane and replace_lane instructions when they could have been a single shuffle. Differential Revision: https://reviews.llvm.org/D100018
In the final SIMD spec, there is only a single v128.any_true instruction, rather than one for each lane interpretation because the semantics do not depend on the lane interpretation. Differential Revision: https://reviews.llvm.org/D100241
This test was disabled despite the instruction having been implemented for a long time. This commit just enables the test. Differential Revision: https://reviews.llvm.org/D100345
Now that these instructions are no longer prototypes, we do not need to be careful about keeping them opt-in and can use the standard LLVM infrastructure for them. This commit removes the bespoke intrinsics we were using to represent these operations in favor of the corresponding target-independent intrinsics. The clang builtins are preserved because there is no standard way to easily represent these operations in C/C++. For consistency with the scalar codegen in the Wasm backend, the intrinsic used to represent {f32x4,f64x2}.nearest is @llvm.nearbyint even though @llvm.roundeven better captures the semantics of the underlying Wasm instruction. Replacing our use of @llvm.nearbyint with use of @llvm.roundeven is left to a potential future patch. Differential Revision: https://reviews.llvm.org/D100411
Add a custom DAG combine and ISD opcode for detecting patterns like (uint_to_fp (extract_subvector ...)) before the extract_subvector is expanded to ensure that they will ultimately lower to f64x2.convert_low_i32x4_{s,u} instructions. Since these instructions are no longer prototypes and can now be produced via standard IR, this commit also removes the target intrinsics and builtins that had been used to prototype the instructions. Differential Revision: https://reviews.llvm.org/D100425
Removes the builtins and intrinsics used to opt in to using these instructions and replaces them with normal ISel patterns now that they are no longer prototypes. Differential Revision: https://reviews.llvm.org/D100402
Use the target-independent @llvm.fptosi and @llvm.fptoui intrinsics instead. This includes removing the instrinsics for i32x4.trunc_sat_zero_f64x2_{s,u}, which are now represented in IR as a saturating truncation to a v2i32 followed by a concatenation with a zero vector. Differential Revision: https://reviews.llvm.org/D100596
This round is dependant on rust-lang/llvm-project#101 landing first in rust-lang/rust and won't pass CI until that does. That PR, however, will also break wasm CI because it's changing how the wasm target works. My goal here is to open this early to get it out there so that when that PR lands in rust-lang/rust and CI breaks in stdarch then this can be merged to make CI green again. The changes here are mostly around the codegen for various intrinsics. Some wasm-specific intrinsics have been removed in favor of more general LLVM intrinsics, and other intrinsics have been removed in favor of pattern-matching codegen. The only new instruction supported as part of this chagne is `v128.any_true`. This leaves only one instruction unsupported in LLVM which is `i64x2.abs`. I think the codegen for the instruction is correct in stdsimd, though, and LLVM just needs to update with a pattern-match to actually emit the opcode. That'll happen in a future LLVM update.
I think it is worthwhile to ask how many changes (and especially those that aren't soundness or bug fixes) are we comfortable with backporting? In order to support a (currently fairly obscure) new feature of a target? Mostly in context of, say, distributions who will most likely not backport changes like these – these commits are not really necessary to e.g. build Firefox. As well as a related recent "scandal" which in part was sparked by the fact that Rust maintains a fork of LLVM with a couple of backports. I guess then this brings me to a question of: how important is it to get WASM SIMD working right now (as opposed to, say, slightly less than half a year later.) And also please do not interpret this as strong opposition and instead as a minor push-back. |
To add, I almost have no doubts that LLVM will continue seeing a steady stream of fixes and improvements around WASM SIMD support as we get closer to the release of LLVM 13. I suspect that we'll continue seeing a steady stream of assertion failures and similar issues by adopting and exposing WASM SIMD to the end users as early as now. Maybe that's a benefit (issues get ironed out sooner in LLVM) but also a downside in that there will be more time spent on codegen issues that are potentially already known or fixed upstream. |
Nah that's a very reasonable question! My motivation with these changes is that the simd proposal for wasm recently moved to "phase 4" which means in the near future browsers are going to un-gate their implementations and wasm simd will be generally available on the web. I wanted to make sure that Rust's story for wasm simd was "up to snuff" and worked well. Getting stable support in Rust is a very long road and it's one that I wanted to start traveling down sooner rather than later. As for why now of all times, for me it boils down to I've got some spare time and I'm motivated. That may not be the case 6 months from now when LLVM 13 is released. As for what changes are backported, my main target has been LLVM's codegen support for new instructions. I'm trying to not backport many changes which are simply optimizations or tweaks to LLVM internals. Some of the patches have seemingly been interdependent, however, so I've backported some in batches. I'm currently under the impression that almost all Rust/wasm use is done through rustup-distributed compilers. I'm not even sure if distributions even have a wasm target for Rust or a story for distributing Rust targets. Given that impression I generally don't really consider distributions or external deployments of Rust when considering these changes myself. I assume that by the time someone really wants wasm/Rust to work on Debian we'll be long after LLVM 13 and won't need any more patches. I also don't doubt that there's going to be a lot of patches for wasm simd, notably around optimizations. Once, however, we get a commit of LLVM that's able to emit all wasm simd instructions I plan to stop backporting unless there's correctness bugs or issues. At that point I plan to focus attention on stabilization of the APIs and such. LLVM doesn't yet support all wasm simd instructions, there's still one missing which is why I suspect one more backport will be necessary. Finally, I agree that this is an obscure feature. My goal is to take on the burden of maintaining all this myself and not impact others (apart from reviews). If it's causing too many other issues then I can be told to wait for LLVM 13. In other words I've generally been under the assumption that despite this being niche it's fine to push forward so long as it's in alignment with general policies (only backports, nothing rust-fork-specific) and I'm the one in the driver's seat for the changes. |
FWIW, while we're not shipping Rust/wasm in Fedora or RHEL yet, it is under active consideration. So I am a bit concerned that Rust may depend on the exact bundled LLVM for wasm, possibly not supporting system LLVM for that target -- but I don't know enough about it, whether this may already be true in the status quo. |
That makes sense, yeah, but I would be hesitant to hold myself back now because something may be done in the future. I also think that saying "Rust can only supports the LLVM fork we have" naturally isn't quite the right way to capture it. Rather if we want to support stock LLVM 12 and the LLVM fork with wasm support here that's quite possible, it just requires more careful work and planning in the compiler itself. I personally don't see the need at this time to do that work, though. |
Ok rust-lang/rust#84339 has been approved so I think this should be safe to go ahead and land (if others are ok with it). I will prepare the update to happen just after that PR lands (or I can also include it in that PR itself if there are no objections) |
With rust-lang/rust#84339 merged would it be ok to get a go/no-go on this? If this is not merged then I need to take a different route to avoid regressing WebAssembly codegen for saturating float cast intrinsics. |
I don't have strong opinions either way, but I think we should still be careful about introducing changes that we require for the target to be operable. If we compile less efficient code without one of these commits, as is the case for saturating cast, then its fine to backport it IMO (though the motivation for doing so is also not as strong). Given that nobody uses wasm SIMD today, I'm comfortable with backporting the SIMD related commits too, if only to unblock your work. But I think it is important to make absolutely sure that the compiler and the standard library can build without these commits as well, at least when the SIMD functionality is not actively used by the user code. |
At least for me it would be helpful to have a hard line drawn in the sand. Rust has some minimum supported LLVM version, so does that mean that all features for all supported targets must work on that minimum LLVM version? Are non-tier-1 targets like WebAssembly exempt? I personally find it pretty difficult to work with concerns like "but what about distros?" when in reality nothing here will impact anyone because everyone using rust & wasm uses rustup-distributed compilers. I feel like it's just a thought exercise at some point and causes a lot of time to be wasted. So put another way, am I being told to wait for all wasm simd work until LLVM 13? Do I need to update |
I feel like what draws the hard line, currently, is the rustc test suite. I believe we only test WASM with our fork of LLVM and so, by that logic, supporting older LLVM releases for this target would not be necessary. As you say there doesn't seem to be a good reason to strive to do so, either, at least not for wasm and not today. The fact that we do not ensure basic functionality with a pristine current LLVM release seems like an oversight to me, but that question should be considered on its own, outside of this PR. We don't really have anything like a written policy for LLVM backports either, as far as I'm aware. There's only an unwritten guideline that LLVM backports should ideally be those that fix major issues that we can't wait for. There have been exceptions made to this in the past, and this seems like a good candidate for such an exception to me. As thus, I don't believe there's anything that would block your work in terms of hard lines. The only final thought I have in favour of landing this PR right now: any discussion about backwards compatibility can still, and probably should, happen on the PR implementing the functionality on the rustc side. It is only there that it is possible to discuss compatibility concerns (if any) in a concrete and not handwavy manner (as I've been here, my bad). There's also infrastructure for decision making (such as FCP) available there. So +1 from me on landing this (but I can't do so – permissioning is borked somewhere :() |
Can you paint a picture of that different route?
In general, no we can't exempt non-tier-1. I don't know if there's a formal policy statement, but practically speaking, min-LLVM support is mostly for the sake of distros, which definitely care about a wider set of targets than just Rust's tier 1. I can accept that things might not work optimally on older LLVM, and the external-LLVM maintainer might even need to consider bug fixes, but the basic functionality should be there.
This is an early adopter chicken-and-egg, but I can assure you that wider use is coming. |
It's somewhat difficult to see what a different route would precisely be because I don't know what that route even is. To be maximally conservative in where the entire test suite is required to run on all platforms for all supported versions of LLVM, then this would look like:
None of this is really all that hard to do or a showstopper by any means, it's all feasible to do. The problem for me is I don't know what to do. I am unaware of anyone using stock LLVM 12 (or anything below) to interoperate with Rust code. Furthermore LLVM 12's binary representation of WebAssembly SIMD instructions is wrong anyway (the opcodes were more recently renumbered). I don't mind doing work to support more things but I thought that the lack of folks using external LLVM versions for was enough for this to be ok. I'm hesitant to put more work in when the reason is "just because", but if the reason is "well that's the policy deal with it" then that makes sense. I'm definitely trying to make sure Rust is on the early adopter train for this, yeah. Otherwise we're basically at the whims of whatever C/C++/LLVM do. If we don't give feedback to LLVM in a timely fashion when it's developing a new feature then we miss the window entirely before it's finalized in clang and much more difficult to change. If we try to stick to purely released LLVM versions then we greatly reduce the ability to give feedback to LLVM about bugs, features, and designs. I would, for example, like to start an FCP for stabilizing wasm simd in the near future (well I wanted to do this in February so "near" is a relative term). I'm hesitant to do this until all the recent changes to I think in any case @nagisa has a good point though in that this isn't really the right place to talk about all this. This is a niche repository with few subscribers. I can't really open a PR for a submodule update when this hasn't been merged, but I could open an issue too. I just need to know what needs to be done. |
I think
Bootstrap sets env
That's only if we say wasm must be supported on <12, right? Is that feature okay on stock LLVM 12?
It's also unclear to me when it will be safe for me to enable rust-wasm in distro builds, so it would be really nice if we start defining this line, for both our sakes. It would be fine in my case to set this baseline at LLVM 12, even acknowledging that the SIMD stuff isn't ready there. If we have to set a baseline of 13 instead, so be it, but I would like to resolve this uncertainty. |
Er so, do you want me to implement everything? I explicitly avoided thinking about all of this when creating this PR and others about wasm SIMD. I didn't really want to expend all the cycles to figure out everything because it just didn't seem worth it. Do you want me to do that and get everything to work everywhere? |
I think you should at least use |
Sure yeah that works for me, I can be sure to mention the LLVM compatibility issues in a request for FCP or stabilization. |
I've posted a request for fcp-to-stabilize at rust-lang/rust#74372 (comment) |
This round is dependant on rust-lang/llvm-project#101 landing first in rust-lang/rust and won't pass CI until that does. That PR, however, will also break wasm CI because it's changing how the wasm target works. My goal here is to open this early to get it out there so that when that PR lands in rust-lang/rust and CI breaks in stdarch then this can be merged to make CI green again. The changes here are mostly around the codegen for various intrinsics. Some wasm-specific intrinsics have been removed in favor of more general LLVM intrinsics, and other intrinsics have been removed in favor of pattern-matching codegen. The only new instruction supported as part of this chagne is `v128.any_true`. This leaves only one instruction unsupported in LLVM which is `i64x2.abs`. I think the codegen for the instruction is correct in stdsimd, though, and LLVM just needs to update with a pattern-match to actually emit the opcode. That'll happen in a future LLVM update.
This round is dependant on rust-lang/llvm-project#101 landing first in rust-lang/rust and won't pass CI until that does. That PR, however, will also break wasm CI because it's changing how the wasm target works. My goal here is to open this early to get it out there so that when that PR lands in rust-lang/rust and CI breaks in stdarch then this can be merged to make CI green again. The changes here are mostly around the codegen for various intrinsics. Some wasm-specific intrinsics have been removed in favor of more general LLVM intrinsics, and other intrinsics have been removed in favor of pattern-matching codegen. The only new instruction supported as part of this chagne is `v128.any_true`. This leaves only one instruction unsupported in LLVM which is `i64x2.abs`. I think the codegen for the instruction is correct in stdsimd, though, and LLVM just needs to update with a pattern-match to actually emit the opcode. That'll happen in a future LLVM update.
101: No need to run `buildO0defaultPipeline` manually. r=ltratt a=ptersilie Co-authored-by: Lukas Diekmann <[email protected]>
This pulls in more changes to wasm codegen for SIMD in LLVM, namely support for the
v128.any_true
instruction as well as updates to how codegen is expected to work for a number of intrinsics. There's only one more instruction LLVM has yet to implement,i64x2.abs
, but there's a fallback for that at least so the hope is that this is most likely the next-to-last update before we can just wait for LLVM 13 to come out which has all of this bundled.This notably has an impact on rust-lang/rust#84339 where if this were to land we could not update the LLVM submodule in rust-lang/rust until rust-lang/rust#84339 lands. The reason for that is that 05751c8 is a breaking change which removes wasm intrinsics that rustc currently uses.
I wanted to go ahead and post this but I think it's best to hold off on merging until that PR lands first.