This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Add codegen support for fpround, fpextend and converting between signed integer and floating-point
ClosedPublic

Authored by gonglingqin on Jun 30 2022, 2:46 AM.

Diff Detail

Event Timeline

gonglingqin created this revision.Jun 30 2022, 2:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 2:46 AM
gonglingqin requested review of this revision.Jun 30 2022, 2:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 2:46 AM
xen0n added inline comments.Jul 1 2022, 11:19 AM
llvm/lib/Target/LoongArch/LoongArchFloat32InstrInfo.td
17–20

Actually I don't quite get why this needs to be different just for LA64. Isn't the behavior for these two insns identical between LA32 and LA64?

llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
140–142
llvm/lib/Target/LoongArch/LoongArchISelLowering.h
38

movgr2fr is definitely not "FP to int". Maybe this comment should be fixed.

gonglingqin added inline comments.Jul 1 2022, 11:40 PM
llvm/lib/Target/LoongArch/LoongArchFloat32InstrInfo.td
17–20

Yes, but we only use these two SDNodes for LA64. In LA32, we use bitconvert, such as (LoongArchFloat32InstrInfo. td, line 224-227)
GPR -> FPR
def : Pat<(bitconvert (i32 GPR:$src)), (MOVGR2FR_W GPR:$src)>;
FPR -> GPR
def : Pat<(i32 (bitconvert FPR32:$src)), (MOVFR2GR_S FPR32:$src)>;
Thanks.

llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
140–142

Thanks for the suggestion, I will change that.

llvm/lib/Target/LoongArch/LoongArchISelLowering.h
38

Thanks. I will change that.

Address @xen0n's comments.

xen0n added a comment.Jul 2 2022, 12:47 AM

Still not quite sure about the necessity of the _LA64 nodes/pats (is it again because of tblgen intricacies?) but the rest looks okay.

llvm/lib/Target/LoongArch/LoongArchISelLowering.h
42

I'm not quite sure about what the "LA" suffix means. Aren't we obviously LoongArch? Or is this meaning "L(64-bit integer), Any FP precision"? This seems likely until I find both "w" and "l" ftintrz's are pattern-matched from this.

Still not quite sure about the necessity of the _LA64 nodes/pats (is it again because of tblgen intricacies?) but the rest looks okay.

Yes, we want to reduce the intricacies of tblgen and use the name to identify this node only for LA64.

llvm/lib/Target/LoongArch/LoongArchISelLowering.h
42

I had hoped to use "LA" for "LoongArch". Thanks for your reminding, I found this suffix redundant for the FTINT node, I will remove it.

Address @xen0n's comments.

xen0n accepted this revision.Jul 10 2022, 9:25 PM

I don't know if the LA64-specific DAG nodes could actually be removed for further simplicity, but the rest seems good to me. Someone could enlighten me on this.

llvm/lib/Target/LoongArch/LoongArchFloat32InstrInfo.td
25

What does anyext mean? Either this isn't meaningful and should be removed, or an explanatory comment should be around.

llvm/lib/Target/LoongArch/LoongArchFloat64InstrInfo.td
201

nit: "FP" or "Floating-point" for consistency.

This revision is now accepted and ready to land.Jul 10 2022, 9:25 PM
gonglingqin added inline comments.Jul 11 2022, 1:23 AM
llvm/lib/Target/LoongArch/LoongArchFloat32InstrInfo.td
25

Thanks, anyext is not very useful for this node, I will remove it

llvm/lib/Target/LoongArch/LoongArchFloat64InstrInfo.td
201

Thanks, , I will change it.

Address @xen0n's comments.

xen0n accepted this revision.Jul 11 2022, 1:32 AM

Ideally someone else should take a look too.

SixWeining accepted this revision.Jul 13 2022, 12:02 AM
This revision was landed with ongoing or failed builds.Jul 13 2022, 12:27 AM
This revision was automatically updated to reflect the committed changes.