Page MenuHomePhabricator

[SystemZ] Optimize bcmp calls (PR47420)
ClosedPublic

Authored by xbolva00 on Sep 20 2020, 9:19 AM.

Diff Detail

Event Timeline

xbolva00 created this revision.Sep 20 2020, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2020, 9:19 AM
xbolva00 requested review of this revision.Sep 20 2020, 9:19 AM
xbolva00 retitled this revision from [SystemZ] Optimize bcmp calls to [SystemZ] Optimize bcmp calls (PR47420).
jonpa added a comment.Tue, Sep 22, 7:19 AM

Maybe also have the test case from my original patch which transforms a memcmp to a bcmp...?

If it is really ok to pass bcmp to visitMemCmpCall(), I think a comment on that specifies this would be nice, both for visitMemCmpCall() and EmitTargetCodeForMemcmp().

Maybe also have the test case from my original patch which transforms a memcmp to a bcmp...?

If it is really ok to pass bcmp to visitMemCmpCall(), I think a comment on that specifies this would be nice, both for visitMemCmpCall() and EmitTargetCodeForMemcmp().

X86 emits same inline codegen for cmp and memcmp:
https://godbolt.org/z/W4dE1z

Right, I will adjust to make it more clear.

Maybe also have the test case from my original patch which transforms a memcmp to a bcmp...?

There are already many test cases for memcmp->bcmp transform so I dont see a need for it.

xbolva00 updated this revision to Diff 293517.Tue, Sep 22, 11:10 AM
jonpa added a comment.Wed, Sep 23, 1:16 AM

Right, I will adjust to make it more clear.

I still think it would be nice to mention this in SelectionDAGTargetInfo.h (in the comment for EmitTargetCodeForMemcmp()) as well...?

xbolva00 updated this revision to Diff 293839.Wed, Sep 23, 1:01 PM

Right, I will adjust to make it more clear.

I still think it would be nice to mention this in SelectionDAGTargetInfo.h (in the comment for EmitTargetCodeForMemcmp()) as well...?

Added.

jonpa added a comment.Thu, Sep 24, 7:35 AM

This looks ok to me, but let's wait for @uweigand as well.

uweigand accepted this revision.Fri, Sep 25, 7:10 AM

LGTM as well.

This revision is now accepted and ready to land.Fri, Sep 25, 7:10 AM
This revision was automatically updated to reflect the committed changes.