Page MenuHomePhabricator

[LoongArch] Add codegen support for the binary operations
ClosedPublic

Authored by SixWeining on Jun 7 2022, 4:32 AM.

Details

Summary

These binary operations include sub/fadd/fsub/fmul/fdiv. Others ops
like mul/udiv/sdiv/urem/srem would be added later since they depend on
shift and truncate that have not been supported.

Note add has been added in a previous patch.

Reference:
https://llvm.org/docs/LangRef.html#binary-operations

Depends on D127205

Diff Detail

Event Timeline

SixWeining created this revision.Jun 7 2022, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 4:32 AM
SixWeining requested review of this revision.Jun 7 2022, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 4:32 AM
rengolin accepted this revision.Jun 7 2022, 5:30 AM

LGTM, thanks!

Perhaps wait a few days to make sure other people have time to comment, too.

Also, now that the target is in-tree, you don't need to number the series anymore, the patches can be independent and be reviewed as they come.

This revision is now accepted and ready to land.Jun 7 2022, 5:30 AM

LGTM, thanks!

Perhaps wait a few days to make sure other people have time to comment, too.

Also, now that the target is in-tree, you don't need to number the series anymore, the patches can be independent and be reviewed as they come.

Thanks. Let me remove the number before submit.

xry111 added a comment.EditedJun 7 2022, 10:43 AM

Does anyone have any idea about "why the CI is failing"? It seems happening for this batch of LoongArch patches and also one MIPS patch I've reviewed today. I guess something is wrong in the CI system or LLVM codebase.

Does anyone have any idea about "why the CI is failing"? It seems happening for this batch of LoongArch patches and also one MIPS patch I've reviewed today. I guess something is wrong in the CI system or LLVM codebase.

This pre-commit CI isn't trust-worthy. The LLVM codebase relies on live buildbots that test main and warrant reverts if they break, instead of duplicating all tests as pre-commit (too many!) and then get something wrong after merge.

What I do, and I recommend to everyone merging patches to main is to run a build + check-all with the tools you need (llvm only, clang, etc).

If main is failing without your patch and your patch doesn't add any more failures (nor change the existing failures) it should be ok to merge.

xen0n accepted this revision.Jun 8 2022, 11:02 PM

Seems good to me, thanks!

llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
90–95

I would probably use a switch here but this is only a minor nit and bikeshedding. No problem in the current form.

SixWeining updated this revision to Diff 435758.Jun 9 2022, 6:51 PM

Remove the patch number in the title.

SixWeining retitled this revision from [LoongArch 1/n] Add codegen support for the binary operations to [LoongArch] Add codegen support for the binary operations.Jun 9 2022, 6:51 PM
MaskRay accepted this revision.Jun 10 2022, 7:54 PM

These binary operations include add/sub/fadd/fsub/fmul/fdiv.

add has been added by a previous patch.

MaskRay added inline comments.Jun 10 2022, 8:03 PM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
75

const MCPhysReg ArgFPR32s[] = {...} has internal linkage, so static can be avoided.

Address @MaskRay's comments:

  1. Revise the title.
  2. Remove necessary static keywords.
SixWeining edited the summary of this revision. (Show Details)Jun 11 2022, 7:48 PM

Fix clang-format issue.

Address @MaskRay's comment in D127200: use ;; to emphasize non-RUN non-CHECK lines.

adjust the dependency

SixWeining edited the summary of this revision. (Show Details)Jun 13 2022, 6:13 AM
MaskRay accepted this revision.Jun 13 2022, 4:48 PM
xen0n accepted this revision.Jun 15 2022, 11:34 PM

still LGTM after rebase

This revision was landed with ongoing or failed builds.Jun 19 2022, 6:44 PM
This revision was automatically updated to reflect the committed changes.