Page MenuHomePhabricator

[WebAssembly] Custom optimization for truncate
Needs ReviewPublic

Authored by jingbao on Sep 8 2021, 8:07 PM.

Details

Reviewers
tlively
penzn

Diff Detail

Event Timeline

jingbao created this revision.Sep 8 2021, 8:07 PM
jingbao requested review of this revision.Sep 8 2021, 8:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2021, 8:07 PM
tlively requested changes to this revision.Sep 9 2021, 5:02 PM

Can you add tests for this to llvm/test/CodeGen/WebAssembly?

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
2504

It would be good to write out the transformation we are doing in terms of selection dag operations and WebAssembly instructions. I don't know what PACK*SDW and PACK*SWB mean.

This revision now requires changes to proceed.Sep 9 2021, 5:02 PM
jingbao updated this revision to Diff 372424.Sep 14 2021, 1:17 AM

Update comments and add tests

tlively added inline comments.Sep 14 2021, 3:25 PM
llvm/test/CodeGen/WebAssembly/simd-vector-trunc.ll
2

If you change the run line to this, you can use utils/update_llc_test_checks.py to autogenerate the test output, which will make it easier for me to understand the transformation we are doing. Also, it looks like the code is more general than it needs to be to handle this one case. Is there an opportunity to either simplify the code or test more cases here?

jingbao added inline comments.Sep 14 2021, 7:25 PM
llvm/test/CodeGen/WebAssembly/simd-vector-trunc.ll
2

the code is more general than it needs to be to handle this one case

I'm not sure I fully understand what you mean by this. Do you want me to change the function body of @trunc16i32_16i8? Is there detailed suggestions on how to change?

tlively added inline comments.Sep 14 2021, 7:45 PM
llvm/test/CodeGen/WebAssembly/simd-vector-trunc.ll
2

For example, extractSubVector and truncateVectorWithNARROW look like they are meant to work with many different vector types. If that's correct, it would be good to add tests for more vector types. Alternatively, if we only care about a smaller set of types, it would be nice if we could simplify the code and make it less general.

jingbao added inline comments.Sep 14 2021, 7:54 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
2553

Hi Thomas, you're right that the code is meant to handle more types. My initial purpose is to discuss the solution with you and Petr first so I keep the types simple and leaves a TODO here. If you're OK with the solution, I can try to cover more types and more tests.

(Actually I have little experience with LLVM types, it may take me some time to cover more, and any advice on the if conditions here is very welcome)

jingbao updated this revision to Diff 375171.Mon, Sep 27, 1:54 AM

Make it more general

jingbao updated this revision to Diff 375451.Mon, Sep 27, 6:17 PM

clang-tidy

Nice, these tests give me a good sense of what this optimization is doing. The transformation looks good.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
2460

For both this and truncateVectorWithNARROW, it would be good to add comments about what the parameters are for and what will be returned.

jingbao updated this revision to Diff 376166.Thu, Sep 30, 4:58 AM

Add comments and simplify parameters

Hi Thomas, would you please take a look at the newest patch? Thanks!