This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix a regression selecting negative build_vector lanes
ClosedPublic

Authored by tlively on Jan 30 2019, 3:24 PM.

Details

Summary

The custom lowering introduced in rL352592 creates build_vector nodes
with negative i32 operands, but these operands did not meet the value
range constraints necessary to match build_vector nodes. This CL fixes
the issue by removing the unnecessary constraints.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Jan 30 2019, 3:24 PM
aheejin added inline comments.Jan 30 2019, 5:03 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
34 ↗(On Diff #184388)

Is this unnecessary or incorrect because it assumes an unsigned integer? Looks like there are ways to specify signed immediate too: example1 example2

tlively marked an inline comment as done.Jan 30 2019, 5:10 PM
tlively added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
34 ↗(On Diff #184388)

It's not incorrect, per se, but the alternative fix would be to change the custom build_vector lowering logic to mask all the build_vector operands it produces to make them unsigned, matching the expectation here. It seemed simpler to save a few cycles and just remove this predicate.

aheejin added inline comments.Jan 30 2019, 9:29 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
34 ↗(On Diff #184388)

Sorry I don't understand. What I meant was to change the definition of ImmI8 and ImmI16 from unsigned to signed immediates. In that case why do we need to make operands unsigned?

tlively updated this revision to Diff 184622.Jan 31 2019, 3:00 PM
tlively marked 2 inline comments as done.
  • Make ImmI8 and ImmI16 match signed immediates
tlively added inline comments.Jan 31 2019, 3:01 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
34 ↗(On Diff #184388)

Oh I understand. I like that idea.

aheejin accepted this revision.Jan 31 2019, 3:05 PM
This revision is now accepted and ready to land.Jan 31 2019, 3:05 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 3:22 PM