Page MenuHomePhabricator

[WebAssembly] Update comments for non-splat pow2 vector test case
ClosedPublic

Authored by aheejin on Jun 27 2018, 5:25 PM.

Details

Summary

After rL335727, (sdiv X, 1) is treated as a special case, so we can
safely transform 'sdiv's in non-splat pow vectors into 'shr's even when
some of its entries are '1'. The test expectations have been already
fixed in rL335771, but the comments were out of date.

Also changed the filename from vector_sdiv.ll to vector-sdiv.ll to
be consistent with other test file names.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Jun 27 2018, 5:25 PM

As divisor = 1 is a special case, I think it's be better to keep the test and modify the comment + checks to ensure its doing what it should.

Thanks. I'm little not sure if you remember the history, so FYI, this test case was added in D46161 (rL329525) to make sure the case with 1's in a vector should NOT be converted to 'shr's and remain as 'sdiv's, in which case shifting a value by the number of its bit widths becomes undef. But after rL335727, you fixed it so now even vectors with 1's in there can be correctly treated as pow2 non-splat vectors, so this test started failing. So in rL335771 someone else (not me) fixed the expectation to 'shr's, so the purpose this case was added is now unnecessary.

Do you mean, even if the original purpose of this case is not there anymore, it's still better to keep this case after modifying the comments?

Thanks. I'm little not sure if you remember the history, so FYI, this test case was added in D46161 (rL329525) to make sure the case with 1's in a vector should NOT be converted to 'shr's and remain as 'sdiv's, in which case shifting a value by the number of its bit widths becomes undef. But after rL335727, you fixed it so now even vectors with 1's in there can be correctly treated as pow2 non-splat vectors, so this test started failing. So in rL335771 someone else (not me) fixed the expectation to 'shr's, so the purpose this case was added is now unnecessary.

Do you mean, even if the original purpose of this case is not there anymore, it's still better to keep this case after modifying the comments?

I think I've fixed the test for you now at rL335884 - the test now passes again, so you can probably safely keep it. I'd recommend updating the checks to check the (immediate) shift amounts if that is possible - to ensure they match the 3 'real' divisors.

Oh, what I meant by the test started failing is not about rL335821 but rL335727. I wasn't even aware of getSetCCResultType thing when I posted the previous comment.. Anyway then I'll update the comment.

aheejin edited the summary of this revision. (Show Details)Jun 28 2018, 2:25 PM
aheejin updated this revision to Diff 153395.Jun 28 2018, 2:26 PM
  • Revive vector_sdiv.ll and changed its name to vector-sdiv.ll
  • Update comments
aheejin retitled this revision from [WebAssembly] Delete vector sdiv test case to [WebAssembly] Update comments for non-splat pow2 vector test case.Jun 28 2018, 2:30 PM
aheejin updated this revision to Diff 153404.Jun 28 2018, 2:40 PM
  • Remove -elf triple which is not needed anymore
aheejin updated this revision to Diff 153405.Jun 28 2018, 2:43 PM
  • Revive -elf.. this is getting removed in D48744
RKSimon accepted this revision.Jun 29 2018, 6:09 AM

LGTM

This revision is now accepted and ready to land.Jun 29 2018, 6:09 AM
This revision was automatically updated to reflect the committed changes.