This is an archive of the discontinued LLVM Phabricator instance.

Improve WebAssembly vector bitmask, mask reduction, and extending
ClosedPublic

Authored by calebzulawski on May 30 2023, 11:04 PM.

Details

Summary

This is inspired by a recently filed Rust issue noting poor codegen for vector masks (https://github.com/rust-lang/portable-simd/issues/351). I'm new to wasm, so I simply followed the reported issue and the wasm spec.

I'm not sure who to add to review this, so feel free to add others or remove yourself.

Diff Detail

Event Timeline

calebzulawski created this revision.May 30 2023, 11:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2023, 11:04 PM
calebzulawski requested review of this revision.May 30 2023, 11:04 PM
Sp00ph added a subscriber: Sp00ph.May 31 2023, 10:11 AM

Use sext instead of zext (better codegen for vecreduce, and necessary for bitmask correctness)

tlively accepted this revision.Jun 7 2023, 9:13 AM

This looks really good! It's nice to see the improved codegen on the existing tests.

llvm/test/CodeGen/WebAssembly/simd-vecreduce-bool.ll
27–30 ↗(On Diff #527182)

Could we skip this sign extension before the v128.any_true? As long as the high bits of each input lane are zeroed (which I believe they should be), the sign extension doesn't affect the outcome of v128.any_true.

This revision is now accepted and ready to land.Jun 7 2023, 9:13 AM

Thanks! By the way, I don't have commit access to the repo, if you are able to commit for me.

llvm/test/CodeGen/WebAssembly/simd-vecreduce-bool.ll
27–30 ↗(On Diff #527182)

I originally thought this as well, but I think the shifts are inserted because the other lanes are undef instead of zero, if that's possible. I added the case at the end (test_cmp_v16i8) where all bits are known, and the shifts disappear.

Sp00ph added inline comments.Jun 7 2023, 9:41 AM
llvm/test/CodeGen/WebAssembly/simd-vecreduce-bool.ll
27–30 ↗(On Diff #527182)

The high bits of <N x i1> are unspecified, which also previously led to miscompilations on AArch64, see this issue: https://github.com/llvm/llvm-project/issues/62211

I'll go ahead and land this.

llvm/test/CodeGen/WebAssembly/simd-vecreduce-bool.ll
27–30 ↗(On Diff #527182)

Great, thanks for the clarification on that.

srj added a subscriber: srj.Jun 8 2023, 11:11 AM

This change has injected a SIGSEGV failure (apparent null pointer deference) in Halide's WebAssembly codegen. I'm working on providing a repro case for you I can post here and hope to have it soon, but in the meantime, can I ask for a revert, to unbreak downstream code?

srj added a comment.Jun 8 2023, 11:22 AM

FYI, building with an asserts-enabled LLVM, I fail with:

llvm-project/17/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:2770: llvm::SDValue performBitcastCombine(llvm::SDNode*, llvm::TargetLowering::DAGCombinerInfo&): Assertion `NumElts == 2 || NumElts == 4 || NumElts == 8 || NumElts == 16' failed.

Stand by for a repro.

srj added a comment.Jun 8 2023, 11:35 AM

Enclosed is a repro case: if I just run llc breakage.ll I fail in the above manner.

Sorry for the breakage! Taking a look now. If a fix looks nontrivial, I'll revert.

Sorry for the breakage! Taking a look now. If a fix looks nontrivial, I'll revert.

I've already identified the issue, the assert is triggering due to vectors of length 1. In that case, the combine step should be skipped.

Also, I noticed only the modified tests made it into the commit, not the new tests I added. Would you like me to open a new review with the changes? Thanks!

Oops, sorry for missing those tests, Caleb. Yes, opening a new review with the fix and tests would be great. Thanks!

srj added a comment.Jun 8 2023, 1:32 PM

Oops, sorry for missing those tests, Caleb. Yes, opening a new review with the fix and tests would be great. Thanks!

So if I understand correctly, you are working on a fix-forward rather than a revert? (If so, please cc me on the fix-forward review)

Yes, my understanding is that @calebzulawski is working on a fix. Since I don't know what the expected timeline is, I'll revert in the meantime.

I'm hoping to have it ready today, but a revert is fine with me

Since this has been reverted, I'm just updating the complete patch here.

All I changed was allowing unexpected vector lengths to lower as usual. Codegen for unusual vector lengths is still very bad--there should be an additional step somewhere that breaks up bitcasts to legal vector lengths (the X86 codegen does this before MOVMSK, but I can't figure out how).

calebzulawski reopened this revision.Jun 8 2023, 5:36 PM
This revision is now accepted and ready to land.Jun 8 2023, 5:36 PM
tlively accepted this revision.Jun 9 2023, 8:14 AM

Thanks! I'll land this again and make sure to include all the tests this time.

This revision was automatically updated to reflect the committed changes.