This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Lower f128 SETCC/SELECT_CC as libcall if p9vector disabled
ClosedPublic

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

Details

Summary

Custom lower f128 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
8219

comparasion typo?

8234

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
8234

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)Dec 28 2020, 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.EditedJan 19 2021, 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.Jan 20 2021, 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.

qiucf commandeered this revision.Mar 23 2021, 10:32 PM
qiucf edited reviewers, added: steven.zhang; removed: qiucf.
qiucf updated this revision to Diff 334051.Mar 29 2021, 10:50 PM
qiucf retitled this revision from [PowerPC] Lower the SETCC/SELECT_CC as libcall for fp128 with Power9 vector disabled to [PowerPC] Lower f128 SETCC/SELECT_CC as libcall if p9vector disabled.
qiucf edited the summary of this revision. (Show Details)
This comment was removed by steven.zhang.
steven.zhang added inline comments.Mar 30 2021, 7:06 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1235

Please confirm if there is any problem with the pattern like: select_cc f128, f128, v2f64, v2f64. Technical speaking, we need to custom all the legalized type.

qiucf updated this revision to Diff 334631.Apr 1 2021, 2:59 AM
qiucf marked an inline comment as done.

Mark BR_CC as expand.

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

We don't need to mark vector types for select_cc here. Instead, select_cc f128, f128, v4i32, v4i32 would crash because of br_cc not expanded.

steven.zhang accepted this revision.Apr 8 2021, 3:48 AM

LGTM. But please hold on for some days to see if @nemanjai has more comments.

This revision is now accepted and ready to land.Apr 8 2021, 3:48 AM

Sorry, I don't mean to accept as PowerPC now ...

This revision was landed with ongoing or failed builds.Apr 11 2021, 7:33 PM
This revision was automatically updated to reflect the committed changes.