This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Handle undefined lane indices in SIMD patterns
ClosedPublic

Authored by tlively on Oct 9 2018, 5:57 PM.

Details

Summary

Undefined indices in shuffles can be used when not all lanes of the output vector will be used. This happens for example in the expansion of vector reduce operations. Regardless, undefs are legal as lane indices in IR and should be supported.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Oct 9 2018, 5:57 PM
aheejin added a comment.EditedOct 10 2018, 3:09 PM

Could you give an example of what kind of programs would generate undefs in the IR and why we need this? And also I guess it's good to include that in the CL and the commit description.

tlively edited the summary of this revision. (Show Details)Oct 10 2018, 5:44 PM
tlively updated this revision to Diff 169139.Oct 10 2018, 5:50 PM
  • Rebase and update summary in commit message

Could you give an example of what kind of programs would generate undefs in the IR and why we need this? And also I guess it's good to include that in the CL and the commit description.

Done. See new description.

aheejin added inline comments.Oct 12 2018, 2:53 PM
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1032 ↗(On Diff #169139)

Why -1? Is it how undef is represented?

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
194 ↗(On Diff #169139)

Are these unnecessary sequence of masking by and also removed in non-undef cases?

202 ↗(On Diff #169139)

Here the same. Are these sext_inreg also removed in non-undef cases?

232 ↗(On Diff #169139)

Move this pattern to extract_lane category above

test/CodeGen/WebAssembly/simd.ll
133 ↗(On Diff #169139)

i32 undef? For all other replace_undef_ functions too.

aheejin added inline comments.Oct 12 2018, 3:21 PM
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1032 ↗(On Diff #169139)

I think MaskVal sounds confusing. It sounds like it is a mask value (such as 0xff). Can we change it to something else, like, just Val or something?

tlively marked 2 inline comments as done.Oct 13 2018, 10:50 PM
tlively added inline comments.
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1032 ↗(On Diff #169139)

ByteIndex seemed like a nice descriptive name.

1032 ↗(On Diff #169139)

Yes, that seems to be how undef is represented.

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
194 ↗(On Diff #169139)

Yes, see multiclass ExtractLaneExtended, which uses the corresponding patterns in multiclass ExtractPat.

202 ↗(On Diff #169139)

Yes, same as above.

tlively updated this revision to Diff 169589.Oct 13 2018, 10:51 PM
  • Address comments
aheejin accepted this revision.Oct 17 2018, 10:50 AM

LGTM with a nit.

lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1032 ↗(On Diff #169139)

Then one-line comment stating that here -1 means undef would be helpful I think.

This revision is now accepted and ready to land.Oct 17 2018, 10:50 AM
This revision was automatically updated to reflect the committed changes.