Just marking this not ready to keep my queue clean until the comments are addressed.

In D87384#2279886, @jsji wrote:

Why this can NOT be done in DAGCombiner by implementing decomposeMulByConstant target hook?

Yes, this definitely needs a test case.

This broke the PPC LLD bot and the failure has been ignored for 4 days. I believe it should be fixed with 3bc3983f229.

In D86879#2274025, @hubert.reinterpretcast wrote:

We believe that the test is necessarily large. Has the associated bot encountered similar issues before? What was the recommended fix to retain the test in more appropriate environments?

I am very much in favour of such readability improvements. LGTM.

If I'm not mistaken, this patch is dependent on another one. If so, please add the dependency in the review.

Are the mentions of R_PPC64_GOT_TLSGD34 in the description typos of some sort? In the code, there are only mentions of R_PPC64_GOT_TLSGD_PCREL34.

Please mention the relevant sections of the ABI in the description (i.e. where it mentions the 1-byte offset). Other than that and the inline nits, LGTM. Please give @MaskRay some time to have another look before committing.

The fact that we catch all these additional opportunities is great. I don't fully understand all the code in the patch because I am not all that familiar with the scheduler so I'd like @stefanp to have a look at this as well.
I do have some nits inline and I'd like to see some tests that exercise the code paths where we can't reorder instructions to allow the optimization (i.e. cases where we return nullptr from getPLDpcAndLdStPair() as well as cases where we are unable to add the edges in schedHazardInsts()).

In D80713#2262751, @jhenderson wrote:

...

I've run into this sort of problem before. The issue is almost certainly either a) incorrect assumption about host system endianness, meaning that you've incorrectly/inadvertently assumed the host is little endian, or b) assumed a 64-bit system somewhere. a) is almost certainly the issue here, based on both the test output and build bot name (ppc64*be*). I've skimmed the patch, but can't obviously see where the code is going wrong, but the test output for kd-zeroed-gfx10.s suggests it's around bytes 48-52. There may be other issues elsewhere though, since 0 renders the same regardless of size and endianness.

This is definitely needed as it is really overloading the machine. However I can't really approve it as I am not familiar with the Python code here.

LGTM. Sorry for the delay.

Is there a way this test case can somehow be broken up into multiple files? The test case takes a very long time to compile which causes intermittent but frequent failures on one of our bots that runs on a fairly small VM. Most of the failures listed here: http://lab.llvm.org:8011/builders/clang-ppc64le-linux?numbuilds=100 are timeout failures when compiling this test. It seems unfortunate to have to increase the timeout limit on the bot just for this - especially if this can be broken up into multiple files.

LGTM. Thanks for fixing this.

@RKSimon Do you think you'll have a chance to have another look at this some time soon?

LGTM aside from a couple of minor nits.

In D86262#2228156, @fhahn wrote:

In D86262#2228123, @nemanjai wrote:

I agree that the justification provided seems somewhat inadequate - using memset/memcpy as a canonical form and teaching DA to handle it seems quite reasonable. However, there are libraries that are highly tuned wrt. when to call these functions and when not to. So it seems to me perfectly reasonable to provide an option to disable this for such uses. For example, a function may perform a memcpy/memset operation on data that is known to only be called only from range-limited sites. The compiler (without PGO+LTO which are often not an option) cannot know that it should expand such calls in the back end.

The issue you raise seems to be a cost-modeling issue, right? Ideally the compiler would know whether it is faster to use a loop or a memcpy. IIUC you are worried about the cases where the compiler replaces a loop with a memset, but the loop would be faster? Is there a way to improve the cost-modeling? IMO adding options like this potentially leads to papering over cost-modeling issues, rather than addressing the underlying issue. Also, such options tend to now work well together with LTO.

I agree that the justification provided seems somewhat inadequate - using memset/memcpy as a canonical form and teaching DA to handle it seems quite reasonable. However, there are libraries that are highly tuned wrt. when to call these functions and when not to. So it seems to me perfectly reasonable to provide an option to disable this for such uses. For example, a function may perform a memcpy/memset operation on data that is known to only be called only from range-limited sites. The compiler (without PGO+LTO which are often not an option) cannot know that it should expand such calls in the back end.

Other than a couple of minor nits, LGTM. I am so sorry about the delay in reviewing this.

In D85994#2219062, @MaskRay wrote:

First, if this issue is not common enough, I'd rather we don't have a diagnostic at all.

If we do need it, note that *(i - 2); may be out of bounds.

Last, we should avoid pre-built object files. You can synthesize them with yaml2obj. The preferred style is llvm/test/tools/llvm-readobj/ELF/*.test. lld/test/ELF has some yaml2obj tests as well.

LGTM. Thanks for fixing this. Hopefully we can get this committed quickly to bring the LLD bot back to green.

In D84360#2215915, @sfertile wrote:

I believe we can have the optimization relocation on a code sequence like:

paddi ra, sym@pcrel
load/store rt, offset(ra)

If so we should have a test showing we relocate it properly even if we don't optimize it yet.

Split the instruction enums into "Legacy" and "Prefixed" and use the .def file only for functions that access the enums as it simplifies the preprocessor logic.

It is incorrect to relax this relocation if it is on a paddi. Bail out from the optimization if the relocation is on anything but a pld.

Cleaned up the definitions of instructions and addressed other minor comments.

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.