This is an archive of the discontinued LLVM Phabricator instance.

[AARCH64][NEON] Allow to sink operands for aarch64_neon_pmull.
ClosedPublic

Authored by sunho on Jan 21 2022, 10:56 PM.

Details

Summary

This teaches AArch64TargetLowering::shouldSinkOperands to sink the operands of aarch64_neon_pmull intrinsic.

Diff Detail

Event Timeline

sunho created this revision.Jan 21 2022, 10:56 PM
sunho created this object with edit policy "sunho (Sunho Kim)".
sunho requested review of this revision.Jan 21 2022, 10:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 10:56 PM
sunho updated this revision to Diff 402182.EditedJan 21 2022, 11:23 PM

Tidy up testcases.

fhahn added a comment.Jan 22 2022, 2:32 PM

Thanks for the patch! Can the test be stripped down? Does there need an actual loop or would just 2 different basic blocks be enough? I realise the test of a related patch is also similar, but ideally we would have more compact tests.

sunho changed the edit policy from "sunho (Sunho Kim)" to "All Users".Jan 22 2022, 3:19 PM
sunho updated this revision to Diff 402285.Jan 22 2022, 9:34 PM

Make test case compact.

sunho added a comment.Jan 22 2022, 9:35 PM

@fhahn I've shrinked the test case. Could you take a look?

dmgreen added inline comments.Jan 24 2022, 12:33 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12188

This falls through to trying to treat the instruction like an indexed pmul, which isn't an instruction available in AArch64. This could do with a test case and possibly being guarded against.

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll
2

Can you run update_llc_test_checks on the file. It's useful in cases like this to show all the output

27

I think you can likely remove this too.

fhahn added inline comments.Jan 24 2022, 12:58 AM
llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll
9

it would also be good to add a few negative tests, where sinking won't happen, e.g. because there are users in other blocks. Also cases where the other or both shuffles need sinking.

15

it think nowadays this should use poison here

sunho updated this revision to Diff 404260.Jan 29 2022, 6:43 AM
sunho marked 4 inline comments as done.Jan 29 2022, 6:45 AM
sunho added inline comments.
llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll
9

When would sinking not happen? I've read the code, but it seems it's always sinking unless the operand is phi nod .

sunho added a comment.Jan 29 2022, 6:48 AM

@fhahn @dmgreen I've accounted for your comments. Please take a look again!

sunho updated this revision to Diff 404263.Jan 29 2022, 6:58 AM

Remove unused line

fhahn added inline comments.Jan 31 2022, 5:07 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12188

IIUC there's still a test case missing to guard against the original issue mentioned by @dmgreen.

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll
9

You are right, users in different BBs do not limit sinking, because the instructions are duplicated in that case. One case where sinking won't happen is if one operand has a high mask and one a low mask for example. But it might be better to test for something like that in llvm/test/Transforms/CodeGenPrepare/AArch64/sink-free-instructions.ll

13

You could use -asm-verbose=0 to trim down the output a bit.

21

it's slightly easier to read if there's a newline after the terminators.

sunho updated this revision to Diff 404634.Jan 31 2022, 11:28 AM

Account for comments

sunho marked 2 inline comments as done.Jan 31 2022, 12:23 PM
sunho added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12188

I've added a test case that makes sure splat vector is not sunk for pmull in "sink-free-instructions.ll."

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll
9

I've added some negative tests in the file you mentioned.

dmgreen accepted this revision.Feb 3 2022, 12:42 AM

Looks like a good patch, from what I can see. Thanks

LGTM

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12201

You don't need brackets around single statements in llvm. I would personally also invert the condition - if (thing) doathing() seems clearer in intent to me, but I do see that other cases here work the other way arounds already.

This revision is now accepted and ready to land.Feb 3 2022, 12:42 AM
sunho added a comment.Feb 3 2022, 4:40 AM

Thanks! Can you commit on my behalf?

Author name: sunho
Author email: ksunhokim123@gmail.com
Commit message: [AARCH64][NEON] Allow to sink operands for aarch64_neon_pmull.

Sure thing. I'll remove the brackets whilst I do.

This revision was landed with ongoing or failed builds.Feb 3 2022, 8:47 AM
This revision was automatically updated to reflect the committed changes.