Update patch based on dependent D93370 patch.

• Rebase patch in order to update test cases

• Rebase patch
• Add documentation regarding refactored load and store implementation
• Add the PPC::MOF_SubtargetP10 flag if the subtarget has prefixed instructions

LGTM.

Address review comments:

• Update the comment for emitting pli directly
• Remove the case to ReplaceNode() if the resulting node from selectI64Imm() is not a constant.

I saw that the patch, along with the temporary workaround for clang-ppc64le-linux has been reverted. I also saw that you committed https://reviews.llvm.org/rGec575e3b0a462ff7a3d23d0f39a22147606050de without a work around for PPC, although that has also been reverted at this time.
Was your recent patch still problematic for PPC and would you still like us to investigate it?

Overall this LGTM.

• Moved setting FrameIndex alignment flags implementation to D93370 (with added comments from Nemanja's review)

• Create a static function to compute flags for address computation
• Create static function to set alignment flags for FrameIndex
• Update comments

LGTM.

This also LGTM.

Gentle ping.

Thanks for fixing the tests. I've tested this patch on the environment of clang-ppc64le-rhel (https://lab.llvm.org/buildbot/#/builders/57), and there seems to be two failures that still remain:

Failed Tests (2):
  Clang :: OpenMP/linking.c
  Clang :: Driver/msvc-link.c

Update patch based on dependent patch (D95116) update, and update test cases with necessary changes.

Update patch based on dependent patch (D95115) update.

Update patch based on dependent patch (D93370), update newly added test cases, address review on comment updates.

Update patch based on dependent patch (D93370).

Rebase this patch to the latest changes.

This patch is NFC as it is just adding test cases. Update the revision to combine some of the files together and to reflect the final version that will be committed.

Sorry for the delay and thank you for addressing my previous comments. I think this LGTM.

I think this LGTM aside from the minor nit on the test case. I am OK with it being updated on the commit.

In D97575#2603354, @amyk wrote:

Hi,

I believe this commit is causing failures on the following PowerPC buildbots:
https://lab.llvm.org/buildbot/#/builders/19/builds/2678
https://lab.llvm.org/buildbot/#/builders/18/builds/868

An example of the errors I am seeing:

CMake Error at /home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm-project/libcxx/benchmarks/CMakeLists.txt:198 (include):
  include could not find load file:
    AddLLVM
CMake Error at /home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm-project/libcxx/benchmarks/CMakeLists.txt:204 (configure_lit_site_cfg):
  Unknown CMake command "configure_lit_site_cfg".
[ 28%] Building IntrinsicsVE.h...
-- Configuring incomplete, errors occurred!
See also "/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/clang_build/projects/compiler-rt/lib/tsan/libcxx_tsan_powerpc64-bins/CMakeFiles/CMakeOutput.log".
See also "/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/clang_build/projects/compiler-rt/lib/tsan/libcxx_tsan_powerpc64-bins/CMakeFiles/CMakeError.log".
make[3]: *** [projects/compiler-rt/lib/tsan/CMakeFiles/libcxx_tsan_powerpc64-build.dir/build.make:87: projects/compiler-rt/lib/tsan/libcxx_tsan_powerpc64-stamps/libcxx_tsan_powerpc64-configure] Error 1
make[2]: *** [CMakeFiles/Makefile2:35496: projects/compiler-rt/lib/tsan/CMakeFiles/libcxx_tsan_powerpc64-build.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....
[ 28%] Built target ClangARMMveBuiltinCG
[ 30%] Built target LLVMMC
[ 30%] Built target ClangDriverOptions
[ 30%] Built target ClangARMSveSemaRangeChecks
[ 30%] Built target ClangARMSveBuiltins
[ 30%] Built target ClangARMSveBuiltinCG
[ 30%] Built target intrinsics_gen
[ 36%] Built target clang-resource-headers
make[1]: *** [CMakeFiles/Makefile2:37499: projects/compiler-rt/test/tsan/CMakeFiles/check-tsan.dir/rule] Error 2
make: *** [Makefile:10086: check-tsan] Error 2

Do you by any chance have an idea of a solution for this? Otherwise, would it be possible to pull this patch?

I believe this commit is causing failures on the following PowerPC buildbots:
https://lab.llvm.org/buildbot/#/builders/19/builds/2678
https://lab.llvm.org/buildbot/#/builders/18/builds/868

Updated the patch to remove duplicate test cases.

Just a few minor nit comments.

Thanks for answering the questions I previously had on this patch.

In addition to the nit comments, I also have the same question as Stefan for getFPAs64BitIntHi/getFPAs64BitIntLo.

I just have a small question about the patch but overall I think this LGTM.

Commandeering this patch.

Address clang-format comments.

Update patch based on dependent patches, and update a comment when checking for P10 specific flags.

Update patch based on dependent patch changes.

Update patch based on dependent patch (D93370).

Update patch to fix a small issue when setting the Base and Disp for DForms when we have constant that fits in 32-bits.
Previously I used a uint64_t when it should have been a uint16_t.

Update patch to rebase from latest change on D93370.

• Update LIT test cases with prefixed load/store checks.
• Added a PrefixInstrs flag.

• Rebase the patch with the latest trunk.
• Updated alignment checks for the frame index.

Update patch to move the check for if we have a value with an offset that fits into a 34-bit immediate into it's own condition instead of inside an else if.

Update patch to:

• rebase patch
• Update the naming of the variable to Parent

Update patch to rebase with latest trunk.

Update patch to rebase with latest trunk, and rearrange/clean up code slightly.

Rebase patch and also add additional handling for frame index (if the frame index is not aligned, match to an XForm instruction).

A few general comments.

Also LGTM.

Addressed review comments:

• Rename some of the MemOpFlags
• Fixed typo in comments
• Moved variables near their use
• Made some of the methods private

Removed the addition of isIntS32Immediate() - it is no longer needed after addressing the comment of using APInt.

• Updated names of the selection functions in the td patterns
• Address the comment of using APInt when computing address flags
• Removed NFC from the title as there is one test case update that we expect a DSForm in (instead of an XForm instruction)

Thanks for updating. I just noticed one more minor nit.

LGTM. Thanks for the update Albion.

I just have a minor nit, but overall it looks good to me.

I'm really sorry for the delay. LGTM to me, as well.

In D93370#2459137, @shchenz wrote:

I think it is better to explicitly give some reasons why we need this big refactoring, in other words, what's the disadvantage/limitation of legacy implementation? Thank you for the big effort.

Add more comments to functions, make provablyDisjointOr() static, fix pattern in PPCInstr64Bit.td.

@steven.zhang The idea is that this patch should be NFC. All existing load/store test cases should pass with this refactoring. I do think there should be more tests added, perhaps in a follow up patch. What do you think?

Please address the comments regarding the test cases.

Just a minor comment but LGTM overall.

This LGTM.

Minor nit but LGTM

Overall LGTM.

Please also address the clang-format comment.

Aside from the clang-format, LGTM.

Thanks for fixing for tests and formatting. LGTM if there are no other concerns.