Page MenuHomePhabricator

[LoongArch] Do not assert value type in isFPImmLegal
ClosedPublic

Authored by gonglingqin on Sep 28 2022, 2:32 AM.

Details

Summary

This patch fixes the failure of llvm/test/CodeGen/Generic/vector.ll and CodeGen/PowerPC/2007-11-19-VectorSplitting.ll for a LoongArch native build.

Diff Detail

Event Timeline

gonglingqin created this revision.Sep 28 2022, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 2:32 AM
gonglingqin requested review of this revision.Sep 28 2022, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 2:32 AM
xen0n added inline comments.Sep 28 2022, 4:39 AM
llvm/test/CodeGen/LoongArch/vector-fp-imm.ll
3

double dash

8

What part of the expected output does this comment refer to? I can only see the offsets like 4 8 or 12 nicely placed inside the f{ld,st}'s already...

gonglingqin added inline comments.Sep 28 2022, 6:40 PM
llvm/test/CodeGen/LoongArch/vector-fp-imm.ll
3

Thanks, I will change it.

8

For line 142-143 of this file, the expected output is fld.s $fa0, $a2, %pc_lo12(.LCPI1_0). After the functionality is complete, similar scenarios can be optimized by adding a backend pass.

Address @xen0n's comments.

xen0n added inline comments.Sep 28 2022, 8:49 PM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
1829

Thinking about it harder, do we want to inspect the element type of vectors and ensure corresponding ISA extension is present? In which case another similar assert is needed/better.

llvm/test/CodeGen/LoongArch/vector-fp-imm.ll
8

OK I see. Although normally it would be better to clarify the comment here, considering it is going to be a short-lived one anyway I'm fine with it as-is.

gonglingqin added inline comments.Sep 29 2022, 4:10 AM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
1829

Thanks for the suggestion, I will add comments.

Address @xen0n's comments.

xen0n accepted this revision.Sep 29 2022, 4:21 AM

Fair enough. Thanks!

This revision is now accepted and ready to land.Sep 29 2022, 4:21 AM

This also makes CodeGen/PowerPC/2007-11-19-VectorSplitting.ll pass, right? So, the summary is not exact.

This also makes CodeGen/PowerPC/2007-11-19-VectorSplitting.ll pass, right? So, the summary is not exact.

Yes, I will update the summary.

gonglingqin edited the summary of this revision. (Show Details)Sep 29 2022, 7:06 PM

This also makes CodeGen/PowerPC/2007-11-19-VectorSplitting.ll pass, right? So, the summary is not exact.

Yes, I will update the summary.

Better make it clear that the fix is for a LoongArch native build because on other machines these 2 tests are OK.

So it could be This patch fixes the failure of llvm/test/CodeGen/Generic/vector.ll and CodeGen/PowerPC/2007-11-19-VectorSplitting.ll for a LoongArch native build.

This also makes CodeGen/PowerPC/2007-11-19-VectorSplitting.ll pass, right? So, the summary is not exact.

Yes, I will update the summary.

Better make it clear that the fix is for a LoongArch native build because on other machines these 2 tests are OK.

So it could be This patch fixes the failure of llvm/test/CodeGen/Generic/vector.ll and CodeGen/PowerPC/2007-11-19-VectorSplitting.ll for a LoongArch native build.

OK, I will update the summary

gonglingqin edited the summary of this revision. (Show Details)Sep 29 2022, 7:32 PM
This revision was landed with ongoing or failed builds.Oct 7 2022, 11:55 PM
This revision was automatically updated to reflect the committed changes.