This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Use register R0 (ZERO) for constant 0
ClosedPublic

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

Diff Detail

Event Timeline

SixWeining created this revision.Jun 7 2022, 4:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 4:42 AM
SixWeining requested review of this revision.Jun 7 2022, 4:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 4:42 AM
myhsu accepted this revision.Jun 8 2022, 6:43 PM

LGTM

llvm/lib/Target/LoongArch/LoongArchISelDAGToDAG.cpp
35

nit: is this variable used in any other place? could we either put it in the inner local scope or spell it out in-place?

This revision is now accepted and ready to land.Jun 8 2022, 6:43 PM
SixWeining updated this revision to Diff 435401.Jun 8 2022, 6:55 PM

Address @myhsu's comment. Thanks.

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

Codegen improvements are exactly as intended. Thanks!

MaskRay accepted this revision.Jun 8 2022, 11:32 PM
MaskRay added inline comments.
llvm/test/CodeGen/LoongArch/imm.ll
167

Usually simple tests are placed before complex ones.

SixWeining added inline comments.Jun 9 2022, 2:10 AM
llvm/lib/Target/LoongArch/LoongArchISelDAGToDAG.cpp
35

Thanks. Currently this variable is only used once but will be used in ohter place later. Let me spell it out in-place.

llvm/test/CodeGen/LoongArch/imm.ll
167

Yes, I agree. Let me make it be the first one of this file. Thanks.

SixWeining updated this revision to Diff 435766.Jun 9 2022, 7:08 PM
  1. Address @MaskRay's comment by moving imm0 testing to the top of the test file.
  2. Remove the patch number in the title.
SixWeining retitled this revision from [LoongArch 5/n] Use register R0 (ZERO) for constant 0 to [LoongArch] Use register R0 (ZERO) for constant 0.Jun 9 2022, 7:08 PM
xen0n accepted this revision.Jun 9 2022, 7:43 PM
MaskRay requested changes to this revision.Jun 10 2022, 8:27 PM

Depends on D127204

The dependency is inverted.

I think this should be placed before other patches.

This revision now requires changes to proceed.Jun 10 2022, 8:27 PM

Depends on D127204

The dependency is inverted.

I think this should be placed before other patches.

The reason why I place it after other patches is that we can obviously see the improvements from the test (introduced by early patches) updates. If this is not good, I can revert the dependency. There is no problem. Thanks.

Depends on D127204

The dependency is inverted.

I think this should be placed before other patches.

The reason why I place it after other patches is that we can obviously see the improvements from the test (introduced by early patches) updates. If this is not good, I can revert the dependency. There is no problem. Thanks.

If you think it's worthwhile, you can mention this in D127204's summary. No need to add unnecessary intermediate steps to the tests to show the effect.

Depends on D127204

The dependency is inverted.

I think this should be placed before other patches.

The reason why I place it after other patches is that we can obviously see the improvements from the test (introduced by early patches) updates. If this is not good, I can revert the dependency. There is no problem. Thanks.

If you think it's worthwhile, you can mention this in D127204's summary. No need to add unnecessary intermediate steps to the tests to show the effect.

That sounds good. Let me mention this in D127204's summary. Thanks.

There are still many changes to sext-zext-trunc.ll

There are still many changes to sext-zext-trunc.ll

Sorry. Maybe I misunderstood your last comment.

To reduce these changes to sext-zext-trunc.ll I should have to revert the dependence, right?

There are still many changes to sext-zext-trunc.ll

Sorry. Maybe I misunderstood your last comment.

To reduce these changes to sext-zext-trunc.ll I should have to revert the dependence, right?

Right. Reverse the dependency.

reverse 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:49 PM

Thanks!

This revision is now accepted and ready to land.Jun 13 2022, 4:49 PM
This revision was landed with ongoing or failed builds.Jun 15 2022, 10:44 PM
This revision was automatically updated to reflect the committed changes.