In D66840#1702109, @jtony wrote:

I am not able to run the last step successfully. Initially, I thought it's because I used wrong password. So I sent another new password hash to Chris Lattner to update it. He updated the password hash for me. When I used the new password to run svn commit -m "Request commit access for jtony" still failed. Anyway know why? Thanks!

In D66840#1702109, @jtony wrote:

I am not able to run the last step successfully. Initially, I thought it's because I used wrong password. So I sent another new password hash to Chris Lattner to update it. He updated the password hash for me. When I used the new password to run svn commit -m "Request commit access for jtony" still failed. Anyway know why? Thanks!

I am not able to run the last step successfully. Initially, I thought it's because I used wrong password. So I sent another new password hash to Chris Lattner to update it. He updated the password hash for me. When I used the new password to run svn commit -m "Request commit access for jtony" still failed. Anyway know why? Thanks!

In rL309966#650647, @tejohnson wrote:

In rL309966#650645, @jtony wrote:

I may not understand the implementation completely. So could you explain a little bit how this patch only affect peeling for full unrolling? It seems to me you disable peeling for all the unrolling (full, partial or run-time). i.e, where is the code that you distinguish peeling according to unrolling type? I cannot find that logic. Thanks a lot!

Because we only pass in false for AllowPeeling in LoopFullUnrollPass::run. In LoopUnrollPass::run (which handles partial and runtime peeling), we pass in None to the Optional AllowPeeling parameter, which means it gets ignored.

I may not understand the implementation completely. So could you explain a little bit how this patch only affect peeling for full unrolling? It seems to me you disable peeling for all the unrolling (full, partial or run-time). i.e, where is the code that you distinguish peeling according to unrolling type? I cannot find that logic. Thanks a lot!

As you mentioned some registers defined by ORIS/ANDIS/XORIS were identified as already sign-extended. But, it is not true for sign extension depending on the immediate (while it is ok for zero extension). Can you provide some details (maybe in a comment) about how they depend on the immediate? This would be helpful for future maintainers.

LGTM.

Address comments from Ecric and Nemanja.

In D40497#938210, @echristo wrote:

How are we generating these isels if they're basically nops?

For the Unexpected Failures: 29, do they still fail without your patch?

LGTM.

In D39163#908211, @MaskRay wrote:

@jtony I have renamed Extend Shift to Bytes0123 Bytes3012 ... because I think the new names are clearer.

32-bit __builtin_bitreverse needs a similar change and also I think there is a missing optimization:

def DWBytes0123 {

dag Word = (i32 (EXTRACT_SUBREG (RLDICL DWSwapInByte.Swap4, 32, 32), sub_32));

}

This could be msvsrld (lower doubleword) if Power ISA 3.0 is available. The original fix was my first revision and I did not expect it would take so much to time to fix a bug... I don't know much about .td but I think the`[IsISA3_0]` optimization and bitreverse32 deserve revisions of their own?

Address the comments from Nemanja.

This patch now LGTM.

In D38705#898743, @MaskRay wrote:

Added comment suggest by jtony.

The test can be updated to something like

for (int i = 0; i < NUM; ++i) {
  sum1 += ReverseBits32(i);
  sum2 += ReverseBits64(i);
}
for (int i = 0; i < NUM; ++i) {
  sum1 -= __builtin_bitreverse32(i);
  sum2 -= __builtin_bitreverse64(i);
}
return sum1 == 0 && sum2 == 0 ? 0 : 1;

But I use git and the document does not mention how to use arc diff to update a file that is in another repo (./projects/test-suite/)

% find . -name .git
./.git
./projects/libcxx/.git
./projects/test-suite/.git
./projects/compiler-rt/.git
./projects/libcxxabi/.git
./tools/clang/.git
./tools/clang/tools/extra/.git

This is my omission in the implementation. All the 1-bit, 2-bit, 4-bit and bytes are swapped, except the high word and low word is not swapped, which is the root cause for the problem. The reason why the test case : projects/test-suite/SingleSource/Benchmarks/Misc/revertBits.c didn't catch it was probably LLVM OPT optimizes ReverseBits64(__builtin_bitreverse64(i)) to just i, so it never tests the code I added (Note I retest it, it generates the same result with gcc at -O2 but generates wrong result without optimization). I also did some other functional testing on my own machine before committing, but I used the number 0x5555555555555555, which was not general enough, so it also didn't catch this subtle bug. I should have used a more general test case. I suggest modifying the test case in the Benchmark (https://reviews.llvm.org/D35188) to be stronger to catch this (like instead of add ReverseBits64(__builtin_bitreverse64) to sum, add ReverseBits64 and __builtin_bitreverse64 separately to the sum so LLVM OPT couldn't recognize it is a reversal of a reversal.

Address comments from Nemanja.
(1) Abstract the common part code for handling D-Form and X-Form LD/ST heuristic into a function.
(2) Fix missing predicate guards (HasP8Vector, AddedComplexity=400) problem.

In D38099#876879, @davide wrote:

Thanks for your contribution.
Some notes:

1. Can you please update the patch to include context (see the doc) ?
2. Can you please add a test? I attached one to the PR.
3. Can you please explain the rationale behind this fix? I'm afraid it's just papering over the problem rather than solving it.

Address all the comments from Nemanja.

Delete the p9-instrs.txt file, since all the needed instructions in it have been implemented.

Address comments from Hiroshi and rebase on trunk.

Refactor the implementation according to Nemanja's suggestion (get rid of the large lambda function replaceLiWithAddi, and inline replaceAddWithCopy)

Address the comments from Nemanja.

Address one more new comment from Hal (add check for compilers that don't support __has_builtin)

Address comments from Hal Finkel.

Kindly ping.

(1) Also test builtin_bitreverse32 and builtin_bitreverse64.
(2) Add the missing newline at the end of the second printf.

Address comments from Hiroshi.

(1) Refactor the messy red zone guard conditions to make it more readable.
(2) Differentiate the red zone behavior for DarwinABI and Non-DarwinABI (SVR4ABI).
(3) Address comments from Hal.