Page MenuHomePhabricator

[CodeGen] Add sources to isVectorClearMaskLegal. NFC.
Needs ReviewPublic

Authored by HanKuanChen on Nov 8 2022, 5:21 AM.

Details

Summary

Targets may require source information to know whether a mask is legal.
(e.g., whether sources are from a same vector). But current
isVectorClearMaskLegal only provides mask and value type, which is not
enough.

Because DAG.getBitcast may add use number of operand (which blocks some
optimization), we will remove the bitcast node if the node is not used.

Diff Detail

Event Timeline

HanKuanChen created this revision.Nov 8 2022, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 5:21 AM
HanKuanChen requested review of this revision.Nov 8 2022, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 5:21 AM

Do you have an upstream target that actually requires this?

Do you have an upstream target that actually requires this?

Well, actually I want to modify isShuffleMaskLegal so that RISCV target can recognize same vector_shuffle patterns if the source are from a same vector (or one of input is undef).
But in some targets (X86 and AArch64), isVectorClearMaskLegal will call isShuffleMaskLegal. If we modify isShuffleMaskLegal first, the implementations in isVectorClearMaskLegal will be very strange.

RKSimon added inline comments.Nov 8 2022, 6:58 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
23166

How nasty would it be NOT to add the bitcast here at all and allow the N0/N1 operands into isVectorClearMaskLegal to keep their original types as long as they can safely be bitcasted to IntVT?

We'd still bitcast it as part of the getVectorShuffle call of course.

spatel added a comment.Nov 8 2022, 7:31 AM

Would the problem be solved if we pulled this code:
https://github.com/llvm/llvm-project/blob/aa37342b3b548a75a4d8ce330f58a361a9ce22f4/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L22893
into a helper that could be used to see the simpler mask before it is actually created in a new shuffle node?

I was thinking about a similar utility in IR, so we don't have to worry as much about non-canonical masks in IR vector passes (this came up in D137341).

HanKuanChen added inline comments.Nov 8 2022, 8:48 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
23166

How nasty would it be NOT to add the bitcast here at all and allow the N0/N1 operands into isVectorClearMaskLegal to keep their original types as long as they can safely be bitcasted to IntVT?

Is the reason we do this because we don't want a DAG.RemoveDeadNode in the end?
I wonder will this make people confused since it violates the vector_shuffle interface (return and inputs should have same type).

RKSimon added inline comments.Nov 8 2022, 9:00 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
23166

Mainly I'm expecting this to go wrong in the future :-( I'm not sure we should be encouraging creation/deletion of temporary nodes like this just for an analysis call.