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

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

Why -1? Is it how undef is represented?

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
194

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

202

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

232

Move this pattern to extract_lane category above

test/CodeGen/WebAssembly/simd.ll
133

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

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

ByteIndex seemed like a nice descriptive name.

1032

Yes, that seems to be how undef is represented.

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
194

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

202

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

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.