This is an archive of the discontinued LLVM Phabricator instance.

[X86] Invert a vector select IR canonicalization with a binop identity constant
AbandonedPublic

Authored by LuoYuanke on Feb 7 2022, 12:33 AM.

Details

Summary

This patch follows https://reviews.llvm.org/D118644 to invert fmul and
fdiv in X86 backend when AVX512 is available.

Diff Detail

Event Timeline

LuoYuanke created this revision.Feb 7 2022, 12:33 AM
LuoYuanke requested review of this revision.Feb 7 2022, 12:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2022, 12:33 AM
pengfei added inline comments.Feb 7 2022, 1:10 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
48950

/

49012–49015

Equals to return combineBinopWithSelect(N, DAG, Subtarget)?

53889

Or call combineBinopWithSelect directly?

llvm/test/CodeGen/X86/vector-bo-select.ll
345–346

Curiously: it should be equal to

vmulps %zmm2, %zmm1, %zmm1 {%k1}
vmovaps %zmm1, %zmm0

Why sometimes we use this way, sometime another?

LuoYuanke added inline comments.Feb 7 2022, 1:41 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
48950

Sorry, I don't understand this comment.

49012–49015

OK, I'll update it.

53889

I prefer to following the current coding convention, so that when there is more to combine we can extent the code in the sub-function.

llvm/test/CodeGen/X86/vector-bo-select.ll
345–346

I guess it is because sometime it is commuted or swapped, sometimes it is not.

pengfei added inline comments.Feb 7 2022, 1:43 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
48950

X / 1.0 --> X

LuoYuanke added inline comments.Feb 7 2022, 1:47 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
48950

Got it. :)

LuoYuanke updated this revision to Diff 406353.Feb 7 2022, 1:49 AM

Address Phoebe's comments.

spatel added a comment.Feb 7 2022, 8:44 AM

Thanks for working on this.
I have a patch to move the existing code over to DAGCombiner with a TLI hook that is only enabled for x86 currently, so it would be effectively NFC.
That would allow us to avoid adding code for custom combining fmul/fdiv to x86 (just add cases for the new opcodes in DAGCombiner).
I'm not sure if one way is more efficient than the other, but let me post that for review.

spatel added a comment.Feb 7 2022, 9:11 AM

See D119150.

I think this patch is good either way, but there would be less diffs if we move the existing code over as a first step.

See D119150.

I think this patch is good either way, but there would be less diffs if we move the existing code over as a first step.

Ok, we can improve more operator based on D119150. I'll abandon this patch.

LuoYuanke abandoned this revision.Feb 7 2022, 6:37 PM
spatel added a comment.Feb 8 2022, 7:55 AM

Added FMUL/FDIV to the updated code here:
905abc5b7db2 (test diffs are identical)

Added FMUL/FDIV to the updated code here:
905abc5b7db2 (test diffs are identical)

Thanks, Sanjay.