This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] SIMD comparisons
ClosedPublic

Authored by tlively on Sep 6 2018, 5:36 PM.

Details

Summary

Match the ordering semantics of non-vector comparisons. For
floating point comparisons that do not correspond to instructions, the
tests check that some vector comparison instruction was emitted but do
not care about the full implementation.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Sep 6 2018, 5:36 PM
aheejin added inline comments.Sep 7 2018, 9:51 AM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
149 ↗(On Diff #164331)

When do you use simdop and when do you use baseInst? Do you use it interchangeably? If so we should use the same variable name I guess. (In previous patches too.)

169 ↗(On Diff #164331)

I can see why you added this class, but that some instructions use this and the others bypasses this and directly use SIMDConditionInt and SIMDConditionFP directly looks a bit confusing to me. Even if we delete this and manually add _u instructions below, it's gonna be only (?) +8 lines, so I guess it'd be easier to read that way..?

test/CodeGen/WebAssembly/comparisons_simd.ll
1 ↗(On Diff #164331)

How about renaming this to simd-comparisons.ll to match other file name conventions you've added so far?

tlively updated this revision to Diff 164499.Sep 7 2018, 1:26 PM
  • Remove unnecessary layer of multiclasses
tlively added inline comments.Sep 7 2018, 1:26 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
149 ↗(On Diff #164331)

I try to use baseInst when the multiclass will add constants to it to create new opcodes and simdop when it will be used as-is. I will go through and make sure this usage is consistent. I would also be fine with changing them all to simdop if you think that would make more sense.

test/CodeGen/WebAssembly/comparisons_simd.ll
1 ↗(On Diff #164331)

I named it this way to be consistent with comparisons_f32.ll, comparisons_i32.ll and friends. Those files should probably be renamed for consistency in a separate CL as well. What do you think of comparisons-i32.ll, comparisons-simd.ll, etc?

aheejin added inline comments.Sep 7 2018, 3:23 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
149 ↗(On Diff #164331)

Oh, I see. The usage makes sense I think.

test/CodeGen/WebAssembly/comparisons_simd.ll
1 ↗(On Diff #164331)

Hmm, yeah, they look like the only ones with underscores in file names. I think renaming them also is a good idea. But other SIMD-related test files all start with simd-, like simd-arith.ll or simd-conversions.ll, so I guess putting simd first makes more sense. Wait, there's an exception, there are exceptions that start with offset-. :facepalm: But anyway.

tlively updated this revision to Diff 164538.Sep 7 2018, 4:13 PM
  • Rename test file for consistency
aheejin added inline comments.Sep 7 2018, 4:18 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
309 ↗(On Diff #164499)

Could you elaborate what this comment means and why we need this only for floats?

tlively updated this revision to Diff 164727.Sep 10 2018, 12:34 PM
tlively marked 2 inline comments as done.
  • Add comment explaining don't care float comparison lowering
aheejin added inline comments.Sep 10 2018, 3:23 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
309 ↗(On Diff #164499)

I don't really understand what "Lower don't float comparisons" means... Did you mean "Don't lower float comparisons?"

tlively updated this revision to Diff 164780.Sep 10 2018, 6:02 PM
tlively marked an inline comment as done.
  • Fix editing mistake
aheejin added inline comments.Sep 10 2018, 6:32 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
348 ↗(On Diff #164780)
  1. Shouldn't "NE" be "ONE" here, according to the comments?
  2. What kind of source pattern does this rule cover? I test your patch with commenting this part out, and all you tests succeed anyway.
tlively updated this revision to Diff 164788.Sep 10 2018, 7:22 PM
  • Remove unnecessary float compare patterns
tlively updated this revision to Diff 164789.Sep 10 2018, 7:35 PM
  • Unroll newly shortened and complicated loop
tlively added inline comments.Sep 10 2018, 7:36 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
348 ↗(On Diff #164780)
  1. No, these aren't the ordered or unordered comparison modes, these are the "don't care" comparison modes, which are allowed to be either ordered or unordered. These patterns say that these "don't care" comparisons should be lowered to whichever comparison type wasm natively supports.
  1. I tried commenting this section out and the tests failed, but I was able to cut the set of comparisons here down to just eq and ne. I'm not sure why it would have passed for you. The "don't care" float comparisons are not expressible in LLVM IR, but are emitted as isel dag nodes in the target-independent automatic expansion of fcmp ueq and fcmp one, respectively.
aheejin accepted this revision.Sep 11 2018, 2:26 PM
aheejin added inline comments.
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
342 ↗(On Diff #164789)

LGTM with one nit: wrap comment around 80 col

This revision is now accepted and ready to land.Sep 11 2018, 2:26 PM
This revision was automatically updated to reflect the committed changes.