This old test case will be failed for the latest code, so I have updated the test case.

Update the patch to remove the info about PGO.

Modify the comments.

@nemanjai @hfinkel I have updated the patch to align to 32 bytes even if wthout PGO data.

Updated the patch to loop 32 bytes for innermost hot loop even if there is no PGO data.

In D61228#1506487, @nemanjai wrote:

Okay, but we just call MBB->getParent()->getFunction().hasProfileData(), where do we actually check that the loop is hot?

Also, even if we align the majority of loops, how much does that really cost us? The code-size impact could be minor compared to the perf improvement, and if so, we should just always do it. It is still true that most users don't use PGO.

Short story: yes, I agree that we should probably just do this regardless of PGO if we don't see any significant performance regressions on important benchmarks.

TL; DR;
The hotness of the loop is checked in MachineBlockPlacement::alignBlocks(). I think the concern with aligning all loops statically determined to be "hot" to 32-bytes runs the risk of a pathologically bad case such as the following:

for (int i = 0; i < HugeValue; i++) {
  // Enough instructions to make the inner loop fall one instruction past a 32-byte boundary
  for (int j = 0; j < UnpredictableValueHighlyLikelyToBeZero; j++)
    // Do something short
}

Such a case would end up with 7 nops to align the inner loop which would presumably tie up dispatch slots. All that being said, I just ran an experiment with exactly that pathological case and the performance degrades by about 1% (which may even be in the noise).

In D61228#1506487, @nemanjai wrote:

Okay, but we just call MBB->getParent()->getFunction().hasProfileData(), where do we actually check that the loop is hot?

Also, even if we align the majority of loops, how much does that really cost us? The code-size impact could be minor compared to the perf improvement, and if so, we should just always do it. It is still true that most users don't use PGO.

Short story: yes, I agree that we should probably just do this regardless of PGO if we don't see any significant performance regressions on important benchmarks.

TL; DR;
The hotness of the loop is checked in MachineBlockPlacement::alignBlocks(). I think the concern with aligning all loops statically determined to be "hot" to 32-bytes runs the risk of a pathologically bad case such as the following:

for (int i = 0; i < HugeValue; i++) {
  // Enough instructions to make the inner loop fall one instruction past a 32-byte boundary
  for (int j = 0; j < UnpredictableValueHighlyLikelyToBeZero; j++)
    // Do something short
}

Such a case would end up with 7 nops to align the inner loop which would presumably tie up dispatch slots. All that being said, I just ran an experiment with exactly that pathological case and the performance degrades by about 1% (which may even be in the noise).

@hfinkel , do you have any other comments?

In D61228#1482324, @hfinkel wrote:

For some special cases, the performance can improve more than 30% after adding the patch for ppc.

Any significant regressions?

In D60811#1472361, @efriedma wrote:

I guess that's right, technically; "getScalarSizeInBits() / 8" will round down, and getStoreSize() will round up. But still, please fix the code so it's clear that check is happening.

This path has been abandoned, the new patch for this bug is in https://reviews.llvm.org/D60811.

Add mtriple=powerpc64le-unknown-unknown for the test files.

@jsji , I will talk with @sarveshtamba to check and add the test case.

Modify the comments to follow reviewer's comments..

Update the patch.

Have checked the missing files.

Add the test file.

Have modifed the comments.

Modify the comments to follow the reviewers' suggestions.

Put if (!isPatchPoint) within if (isSVR4ABI && isPPC64) to follow nemanjai's suggestion.

In D56148#1342250, @hfinkel wrote:

So this rell-request

typo in the summary "rell"

Modify the spelling erros.

Add the comments for PromoteIntRes_ADDSUBCARRY.

In D55977#1339258, @hfinkel wrote:

In D55977#1339255, @hfinkel wrote:

LGTM

Also, if there's a way to have a non-asserts-required test case, that would be useful. How did you notice the problem?

Add space on the right of !=. So that the Kind !=SelectSupportKind::ScalarCondVectorVal will be modified to Kind != SelectSupportKind::ScalarCondVectorVal.

Because the ELFv1 is different with ELFv2, so I add the guard before use GCCAddlRegNames.

Add the comment for the array AddlRegName to tell about how these RegNum encoding are defined.

@jsji I have updated a new patch for fix the error VSX register index mapping of the old patch.

Fix the VSX register index mappping of the array GCCAddlRegNames.

@jsji , I have uploaded a new patch to avoid renaming from vs32 to` v0` in clobber list.

The old patch will rename from vs32 to` v0` in clobber list, that‘s is not very reasonable.
The new patch will override getGCCAddlRegNames to map vs* to reg numbers, this can avoid unnecessary renaming from vs32 to` v0` in clobber list.