Page MenuHomePhabricator

[NFC][PowerPC] Refactor the tryAndWithMask()
AcceptedPublic

Authored by steven.zhang on Jan 6 2020, 2:28 AM.

Details

Reviewers
nemanjai
jsji
amyk
NeHuang
shchenz
Group Reviewers
Restricted Project
Summary

This NFC patch is to address to comments of https://reviews.llvm.org/D71693 to split the tryAndWithMask into several small calls.

Diff Detail

Unit TestsFailed

TimeTest
10 msLLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; llvm-go test llvm.org/llvm/bindings/go/llvm

Event Timeline

steven.zhang created this revision.Jan 6 2020, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 2:28 AM
lkail set the repository for this revision to rG LLVM Github Monorepo.Jan 6 2020, 7:08 PM
amyk added a comment.Jan 17 2020, 3:46 PM

Just some minor comments, I think it overall LGTM but am curious in knowing any comments from others.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4386

Just curious, but any reason on why we split up the declaration of variables? I mean since the original had Imm, SH, MB, ME declared together?

4400

Unnecessary whitespace change between { } (also seen in other places for SDValue Ops[] brackets as well)?

4429

I think it would be more clear if it is read, ". . . replace the contents of [0 - 32) with [32 - 64) even if we didn't rotate it.

4767–4768

I think it would still be valuable to have a comment here to document this case with AND.

steven.zhang marked 2 inline comments as done.Sun, Feb 9, 5:57 PM
steven.zhang added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4386

Personally, I prefer to move the declaration to the places that where it is really needed to make it more clear instead of declare all the declaration first.

4400

That was done by clang format. I will update other places that has extra spaces.

steven.zhang added a project: Restricted Project.

Address comments.

shchenz accepted this revision.Sun, Feb 16, 6:51 PM

LGTM. Thanks for refactoring this.

This revision is now accepted and ready to land.Sun, Feb 16, 6:51 PM

I will hold on for several days in case someone else has more comments.