This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly][NFC] Enable explicit locals in simd-arith.ll
AbandonedPublic

Authored by tlively on Sep 10 2018, 3:15 PM.

Details

Reviewers
aheejin
dschuff
Summary

The tests for an upcoming CL break when explicit locals are disabled
for this file.

Diff Detail

Event Timeline

tlively created this revision.Sep 10 2018, 3:15 PM

Which aspect of the upcoming CL breaks it?

Which aspect of the upcoming CL breaks it?

It was the same problem as in https://reviews.llvm.org/D51459 where a TEE is emitted but when explicit locals are disabled it is not properly converted to a TEE_LOCAL. The fact that a TEE is emitted isn't actually interesting in the test, but it causes a crash unless it is lowered to a TEE_LOCAL.

How did you resolve it in D51498 then?

I'm not actually sure. Something else must have changed to make it a non-issue then. But the resolution we settled on at that time was to remove the -wasm-disable-explicit-locals flag from the test.

tlively updated this revision to Diff 164781.Sep 10 2018, 6:13 PM
  • Replace constants in generated variable names
aheejin added a subscriber: aardappel.EditedSep 10 2018, 6:37 PM

I'm not actually sure if removing -wasm-disable-explicit-locals or -wasm-keep-registers from test cases is a direction we should head towards, because that generally makes test cases harder to read, as @aardappel in D51459. Especially removing -wasm-keep-registers makes writing test cases extremely hard. Leaving these options as they are and assign 0 as a fake opcode to TEE_LOCAL might be an option too, as I suggested the other day. But that's just how I feel, and I'm not sure if we agreed on gradually removing those options from the test cases. What do you think, @dschuff and @aardappel ?

tlively abandoned this revision.Sep 12 2018, 10:15 AM

As discussed, abandoning this in favor of moving encoding tests to a separate file.

@aheejin I was originally of the opinion that we should move towards removing these flags as much as possible, but I understand that may not feasible. I guess we should do whatever makes sense for the test.

My bigger motivation was to make the MC representation cleaner: we currently have the situation that MC is registerless.. unless these flags are used.