This is an archive of the discontinued LLVM Phabricator instance.

Do not lower VSETCC if operand is an f16 vector
ClosedPublic

Authored by pirama on Dec 8 2015, 4:33 PM.

Details

Summary

SETCC with f16 vectors has OperationAction set to Expand but still gets
lowered to FCM* intrinsics based on its result type. This patch skips
lowering of VSETCC if the operand is an f16 vector.

v4 and v8 tests included.

Diff Detail

Repository
rL LLVM

Event Timeline

pirama updated this revision to Diff 42247.Dec 8 2015, 4:33 PM
pirama retitled this revision from to Do not lower VSETCC if operand is an f16 vector.
pirama updated this object.
pirama added reviewers: ab, jmolloy.
pirama added subscribers: llvm-commits, srhines.
jmolloy requested changes to this revision.Dec 10 2015, 3:05 AM
jmolloy edited edge metadata.

Hi Pirama,

I'm slightly concerned that LowerSETCC is being called when the operation action is Expand. Could you please elaborate/investigate why this is the case before we go ahead with this patch?

Cheers,

James

This revision now requires changes to proceed.Dec 10 2015, 3:05 AM

Hi James,

During legalization, custom lowering is called based on the result type of a node and not its operand type. In this particular case, the caller is LegalizeOp in lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp (look for a switch statement close to the end of this function). Since the result type in these cases are i16 vectors, lowering still gets invoked.

As an aside, the legalizer queries OperationAction based on type of operand when legalizing operands and type of result when legalizing results. That is what necessitates these explicit checks/filters.

-Pirama

pirama added a comment.Jan 8 2016, 9:51 AM

Ping... James, can you look at my earlier comment answering your question?

Thanks,
-Pirama

jmolloy accepted this revision.Jan 21 2016, 12:31 AM
jmolloy edited edge metadata.

Hi Pirama,

Sorry for the long response time on this. It LGTM. Thanks for the explanation!

James

This revision is now accepted and ready to land.Jan 21 2016, 12:31 AM
This revision was automatically updated to reflect the committed changes.