This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly][NFC] Finish cleaning up SIMD tablegen
ClosedPublic

Authored by tlively on Dec 23 2020, 10:36 AM.

Details

Summary

This commit is a follow-on to c2c2e9119e73, using the Vec records introduced
in that commit in the rest of the SIMD instruction definitions. Also removes
unnecessary types in output patterns.

Diff Detail

Event Timeline

tlively created this revision.Dec 23 2020, 10:36 AM
tlively requested review of this revision.Dec 23 2020, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2020, 10:36 AM

Nice!

llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
63

Can we do this? (Other places too)

509

Does not using LaneIdx32 change anything? (I guess LaneIdx has range checks but this doesn't?) (By the way it looks it should be LaneIdx16)

526–530
722

How about defining something like

defvar IntVecs = [I8x16, I16x8, I32x4, I64x2];

at the top and use it elsewhere?

725

Any reason SIMDBitwise does not use SIMDBinary anymore?

736–740

Any reason NOT does not use SIMDUnary?

1121–1130

Is it subject to change?

1146–1158

Any reason this does not use SIMDConvert?

tlively updated this revision to Diff 313657.Dec 23 2020, 10:26 PM
  • Reuse vt as suggested
  • Add IntVecs as suggested
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
63

Yes, it turns out we can!

509

If I understand correctly, using LaneIdx32 in the input pattern is sufficient to apply the range checks, so it's not needed in the output pattern as well. (However, imm is still needed because it makes the resulting MachineOperand an immediate rather than a register.)

Shuffle uses LaneIdx32 because the indices can refer to bytes in either of the two input vectors.

722

Good idea, done.

725

The previous version used SIMDBitwise created four different version of the same instruction when all we really needed was one instruction with four different patterns. Since it turns out we can use foreach in a multiclass to generate Pats and use NAME to refer to the instruction in those Pats, this seemed like a good way to get rid of the redundant instruction definitions.

This also lets SIMDBinary get rid of the prefix argument since SIMDBinary no longer needs to generate instructions that start with "v128."

736–740

Similar to the reasons for the previous change. We don't need four different versions of NOT and this lets SIMDUnary get rid of the prefix argument.

1121–1130

These opcodes are not part of a consistent design, but rather chosen from the available opcode space to get prototypes done in V8 as quickly as possible. I'll be a little sad if the opcodes we end up shipping don't have a consistent arrangement, but we haven't discussed whether we are going to do another change yet, so I don't know whether this will change.

1146–1158

Yes, these narrowing operations take two operands but SIMDConvert instructions have just one operand.

aheejin accepted this revision.Dec 23 2020, 11:04 PM

Thanks for the explanation! LGTM % the alignment nit

llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
526–530

ping

This revision is now accepted and ready to land.Dec 23 2020, 11:04 PM
tlively updated this revision to Diff 313893.Dec 28 2020, 1:49 PM
  • Fix alignment
tlively marked an inline comment as done.Dec 28 2020, 1:49 PM
This revision was landed with ongoing or failed builds.Dec 28 2020, 1:59 PM
This revision was automatically updated to reflect the committed changes.