There seems to be some (weak) correlation between this and https://reviews.llvm.org/D84309 which I am reworking to remove it from canonicalization. It may be worth looking into whether we can catch both issues in CGP.

Ping.

It seems like we keep adding more and more complexity here to handle these flags. Is there not a pass we can just run that will simply re-compute this stuff (at least for the use of this while we're in SSA)? What does LiveVariables do? We certainly end up running DCE after the MI peephole to get rid of dead instructions after the transformation. It seems like we should be able to do the same thing for killed/dead flags as well.

Sure, it's PPC specific and the name should reflect that. Thanks. LGTM.

I agree with Victor, please remove uses of future and use pwr10. All of this can be addressed when committing - another round of review is not required. LGTM.

Feel free to address the comment when committing the patch.

• Move implementation completely into LLD
• Add option to disable the optimization
• Handle computation of the total offset when the access instruction has a non-zero displacement
• Add tests with the new option as well as a test with non-zero displacement on the access instructions

Need to account for a displacement on the access instruction and add some tests along the lines of

extern int Ext[40];
int test(int a) {
  return a + Ext[10];
}

In D84570#2190757, @nemanjai wrote:

It would appear that this failure is not really related to this patch at all. Seems to fail with -O0/-O1 regardless of which patch is applied.
https://pastebin.com/1Mvz5KDz

Please let me know if there's something else I can help with.

It would appear that this failure is not really related to this patch at all. Seems to fail with -O0/-O1 regardless of which patch is applied.
https://pastebin.com/1Mvz5KDz

I'll send you the results in a few minutes. Sorry about the delay - holiday here in Canada.

Since we are adding another bot, we need to adjust the number of threads that are used by LIT for all the bots running on that machine. Thanks.

I forgot to add the differential revision to the commit message.
Committed in https://reviews.llvm.org/rGacb3b8dce1cd

Remove erroneous unrelated update.

Fix up usage of unsigned long literal suffixes.
Add new rel-expr.
Fix up minor nits.
Shorten the test cases.

In D84570#2185572, @kubamracek wrote:

CC @simoatze, @wschmidt:

Would anyone with access to a Linux/PPC64 machine be willing to take a look if the open() syscall is receiving the right value (0600 from the testcase)?

Sample of the build bot failure this resolves:
http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/6122/steps/ninja%20check%201/logs/stdio

In D84785#2179616, @nemanjai wrote:

@wschmidt Sorry to ask you to go back to looking at LLVM code, but figured I'd add you for two reasons:

1. If gcc has the same issue
2. If the proposed fix is completely out to lunch :)

LGTM. Thank you.

@wschmidt Sorry to ask you to go back to looking at LLVM code, but figured I'd add you for two reasons:

1. If gcc has the same issue
2. If the proposed fix is completely out to lunch :)

Add test cases for all the different instructions - thanks for your help producing these @NeHuang.
Run clang-format on the patch since the pre-commit build flagged a bunch of issues.

The fix to recover the regression: https://reviews.llvm.org/D84659

LGTM.

In D77448#2165900, @MaskRay wrote:

@nemanjai See https://gist.github.com/MaskRay/ba7547f6a587176666ff77527cf12c4d for a reproduce.

In rG59a76d957a2603ee0548293ded170c98e1308c48#936919, @amyk wrote:

Hi there!
I believe this patch is causing failures on the following (big endian) PowerPC buildbots:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/
http://lab.llvm.org:8011/builders/clang-ppc64be-linux

Example of the failures can be seen at: http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/51729

Could you please fix or pull this changeset if necessary? Thank you.

Sounds like agreement. I'll move this to CGP. Thanks for the quick response.

LGTM. I am really sorry about not getting back to this earlier.

LGTM. The test case addition can be done on the commit.

There is a lot of instances where this warning fires in the unit tests since gtest does not appear to use the override keyword. This in turn causes failures on bootstrap builds that use -Werror.
An example: http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/10717

The remaining comments are minor nits that can be addressed on commit. LGTM.

Also, can you run clang-format so all the notifications go away and make this easier to review?

The description includes ... however it is more preferable to use bitcast. It is not a question of preference but of correctness. The fp to int conversions truncate while bitcasts don't. The semantics of the builtins require that no truncation happen.

Since clang will now add +/-mma to the TargetFeatures list, please add a test case that specifies -mattr=+/-mma to llc to show that llc accepts it.
Other than that, LGTM.

Please re-upload this and provide the missing context.

Seems fine to me, but of course, wait to hear from Sean/MaskRay.

@rzurob This cannot proceed without your approval since you requested changes.

LGTM aside from a minor nit regarding the description.

LGTM. Thanks for fixing this.

LGTM. Thank you.

My remaining comments are minor and can be addressed on the commit. AFAICT, Sean's comments have been addressed as well so I think this is good to go. @sfertile if you have any objections, feel free to override my approval, otherwise this is good to go.

Forgot to select Accept.

Minor nits, otherwise LGTM.

LGTM as long as the inheriting of GCCBuiltin is removed. This can be done on commit.

The remaining updates are straightforward so feel free to address my comments on the commit. LGTM otherwise.

Other than removing the assert, this LGTM.

Pretty close to the way it should be but the indices do need to flip so I have to request changes.

LGTM other than the naming nit.

This fixes it:

diff --git a/lld/lib/ReaderWriter/MachO/CMakeLists.txt b/lld/lib/ReaderWriter/MachO/CMakeLists.txt
index 37d1de4..bdbfbee 100644
--- a/lld/lib/ReaderWriter/MachO/CMakeLists.txt
+++ b/lld/lib/ReaderWriter/MachO/CMakeLists.txt
@@ -24,6 +24,7 @@ add_lld_library(lldMachO
     Demangle
     Object
     Support
+    TextAPI

The shared libs build of LLD is broken and it appears to be as a result of this patch:

tools/lld/lib/ReaderWriter/MachO/CMakeFiles/lldMachO.dir/MachONormalizedFileBinaryReader.cpp.o: In function `lld::mach_o::TAPIFile::doParse()':
MachONormalizedFileBinaryReader.cpp:(.text._ZN3lld6mach_o8TAPIFile7doParseEv[_ZN3lld6mach_o8TAPIFile7doParseEv]+0x60): undefined reference to `llvm::MachO::TextAPIReader::get(llvm::MemoryBufferRef)'
tools/lld/lib/ReaderWriter/MachO/CMakeFiles/lldMachO.dir/MachONormalizedFileBinaryReader.cpp.o: In function `lld::mach_o::TAPIFile::loadFromInterface(llvm::MachO::InterfaceFile&)':
MachONormalizedFileBinaryReader.cpp:(.text._ZN3lld6mach_o8TAPIFile17loadFromInterfaceERN4llvm5MachO13InterfaceFileE[_ZN3lld6mach_o8TAPIFile17loadFromInterfaceERN4llvm5MachO13InterfaceFileE]+0x1dc): undefined reference to `llvm::MachO::mapToArchitectureSet(llvm::ArrayRef<llvm::MachO::Target>)'
MachONormalizedFileBinaryReader.cpp:(.text._ZN3lld6mach_o8TAPIFile17loadFromInterfaceERN4llvm5MachO13InterfaceFileE[_ZN3lld6mach_o8TAPIFile17loadFromInterfaceERN4llvm5MachO13InterfaceFileE]+0x30c): undefined reference to `llvm::MachO::mapToArchitectureSet(llvm::ArrayRef<llvm::MachO::Target>)'

Also, please add a couple of new test in test/CodeGen/PowerPC/p10-splatImm.ll so that when someone looks at this commit in the log, they can see that vec_splati and vec_splatid are meant to produce xxspltiw and xxspltidp (i.e. the same IR produced for the calls in this test, produces the desired instruction)..

If this patch does not include the back end codegen test cases for these, which patch does? I don't see any selection patterns or test cases for xxsplti32dx in ToT.
The implementation and test cases for xxspltiw are in ToT, so they don't appear to be a concern.

LGTM.

LGTM other than the minor nit.

The nits can be addressed on commit. I don't think another review is required. LGTM.

As far as I'm concerned, this is fine for now. We can remove these once all in-tree target have implemented their support.
LGTM but maybe give a couple of days for others to chime in.

LGTM. Thanks.

In D77448#2116050, @nathanchance wrote:

Gentle ping. This is still an issue at 4c6548222b3c41d024581d28f42b3f02510bcfe3 and I have not heard anything about a fix. I know I sent that message on Friday night before the weekend so I am hoping that it did not get lost. If I need to file a bug report, I am more than happy to do so.

This is a nice refactor. The only thing I would like to see is a statement to indicate that you have ensured that PPCGenDAGISel.inc is identical before and after this patch.

In D80175#2081483, @AaronLiu wrote:

Can you add a hidden option with init false? You can turn it true later on.
So that people can try with your option off and on? Thanks!

Oh, apparently Fangrui fixed it in rG81cca9876847.

In rG3b7ec64d59748765990ed99716034ab8d5533673#926929, @gchatelet wrote:

AFAICT this patch is causing the follwing error

LGTM. Thanks Amy.