Page MenuHomePhabricator

[PowerPC] Lower the SETCC/SELECT_CC as libcall for fp128 with Power9 vector disabled
Needs ReviewPublic

Authored by steven.zhang on Nov 25 2020, 2:30 AM.

Details

Reviewers
nemanjai
qiucf
jsji
Group Reviewers
Restricted Project
Summary

Custom lower the setcc/select_cc as libcall if power9 vector is not enabled.

Diff Detail

Event Timeline

steven.zhang created this revision.Nov 25 2020, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2020, 2:30 AM
steven.zhang requested review of this revision.Nov 25 2020, 2:30 AM
qiucf added a comment.Nov 26 2020, 6:22 PM

Thanks for the work! I thought code to 'unfold' SELECT_CC and BR_CC should exist in generic code..

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4475 ↗(On Diff #307548)

Will this okay if IsStrict == true?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7414

comparasion typo?

7429

This early return isn't about P8 or P9?

steven.zhang planned changes to this revision.Nov 30 2020, 3:00 AM

Rebase the patch.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4475 ↗(On Diff #307548)

It is the compare of integer type not floating type.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7429

It is old logic. What I did is just replacing the Op.getOperand(0).getValueType() with CmpVT.

steven.zhang planned changes to this revision.Dec 16 2020, 12:05 AM
steven.zhang retitled this revision from Legalize the SETCC/STRICT_FSETCCS as libcall to [PowerPC] Lower the SETCC/SELECT_CC/BR_CC as libcall for fp128 with Power9 vector disabled.
steven.zhang edited the summary of this revision. (Show Details)

Please explain in the description of the review (and subsequent commit message) why custom lowering is needed. I suppose for SELECT_CC, it is needed because using setOperationAction() uses the result type which is only guaranteed to match the true/false value but not the comparison types. However, it is not immediately obvious to me why we need custom lowering for BR_CC.

steven.zhang edited the summary of this revision. (Show Details)Mon, Dec 28, 6:09 PM

Please explain in the description of the review (and subsequent commit message) why custom lowering is needed. I suppose for SELECT_CC, it is needed because using setOperationAction() uses the result type which is only guaranteed to match the true/false value but not the comparison types. However, it is not immediately obvious to me why we need custom lowering for BR_CC.

Update the description. The reason why we need to custom lower the BR_CC has been explained in the code comment. The legalizer tries its best to expand the BR_CC which is not what we want.

// Lower the br_cc as follows for fp128 as we didn't have native
// instruction to lower the setcc on fp128.
// br_cc cc, x, y, dest ->
// z = setcc x, y, cc (expand as libcall)
// br_cc NE, z, 0, dest

Gentle ping ...

@nemanjai Do you have any more comments on this ? Thank you!

The description isn't really adequate - it does not answer my question at all really. The question is why not set the legalization action for BR_CC as Expand? Namely, what does the legalizer do that we don't want it to do if we let it expand the node? You just answered the question with the equivalent of "The legalizer does its job and that's not what we want." So please explain why that is not what we want.

steven.zhang added a comment.EditedTue, Jan 19, 1:52 AM

The description isn't really adequate - it does not answer my question at all really. The question is why not set the legalization action for BR_CC as Expand? Namely, what does the legalizer do that we don't want it to do if we let it expand the node? You just answered the question with the equivalent of "The legalizer does its job and that's not what we want." So please explain why that is not what we want.

Hmm, we will hit assertion with legalizer as it tries to swap the operand and cc to see if target support it, and assertion otherwise. Basicly, we have two options. One is to enhance the legalizer and another one is to do it inside PowerPC target as what this patch did. I am swing between them. Thank you for bringing this up and I will update the patch to go with direction on enhancing the legalizer and it might answer your question directly.

steven.zhang retitled this revision from [PowerPC] Lower the SETCC/SELECT_CC/BR_CC as libcall for fp128 with Power9 vector disabled to [PowerPC] Lower the SETCC/SELECT_CC as libcall for fp128 with Power9 vector disabled.Wed, Jan 20, 9:36 PM
steven.zhang edited the summary of this revision. (Show Details)

Split the BR_CC into another patch which will enhance the legalizer to support it. For SETCC/SELECT_CC, we will do it inside PowerPC as what other target did.