Aside from a few minor nits, LGTM.

In D71831#1938331, @steven.zhang wrote:

In D71831#1936489, @nemanjai wrote:

2. Rename RotateRightClearLeft to something like RightJustifyRangeAndClear as it appears that is what the function is doing.

In ISA, the RLDICL is named as: "Rotate Left Doubleword Immediate then Clear Left". I am not sure if the right justify range make it more clear. Regarding to left or right, it is just a wrap rotate. The lambda here is trying to hide the detail that implement the right rotate with left rotate. Personally, I prefer the RotateRightClearLeft one, but also ok to the RightJustifyRangeAndClear if you insist.

Ugh...
So we have a transformation that is called foldFrameOffset() and it would actually transform the attached test case? There does not appear to be any FrameIndex in the attached test case. So in addition to a missed check for R0/X0 vs. PPC::ZERO/PPC::ZERO8 we also have a transformation that says it folds frame offsets, but it does not really care about frame offsets - it just folds arithmetic into the index register.

I initially misread the comment from Eli. Fold both splitting-related checks into the new function.

Factored out detection of an opaque target constant index into a static function and updated the test case.

In D76778#1944623, @efriedma wrote:

Please refactor the "HasOTCInc || !MaySplitLoadIndex" condition into a helper function.

In D76240#1944755, @MaskRay wrote:

In D76240#1927115, @MaskRay wrote:
For lld, I think --threads={0,1,2,...} is better than the current --(no-)threads.

I have a change to rename lld --no-threads to --threads=N. It takes more efforts to actually work because I need to patch llvm::parallel::for_each_n called by lld/include/lld/Common/Threads.h#L75

Can the lit test run a system lld which may not support --threads= after --no-threads is removed?

LGTM. Thanks.

Added comments explaining why threads are disabled in LLD when it is the default linker.

The author of the PR confirms that the patch fixes the problem.
Also, the failing unit tests are not due to this patch.

In D76240#1942048, @efriedma wrote:

Adding a flag like that requires a comment explaining the need each place you're adding it.

Is there some way to refactor this so the code isn't scattered across five Python files?

Seems that people have lost interest in this review, but I still have the problem on my LLD builds. Any further comments from anyone?

LGTM. Thanks.

Thanks for putting this up. This is actually something I meant to do very soon as we need it for a specific benchmark. Please fix the pattern and then this patch is fine.

Ah, OK. This makes sense.
I realize that it is not really possible to write a test case to show the allocation of R0 previous to this patch. However, would you modify existing test cases that use regex matching for the target register such as [[REG:[0-9]+]] to something like [[REG:[1-9][0-9]*]] to demonstrate that R0 is not a valid candidate for the regex match?

This seems perfectly fine with me - and more consistent with GNU binutils.
However, I think the AIX guys should also have a look as I am not sure what the system assembler's capabilities are on AIX. I'll add a couple of AIX reviewers.

LGTM aside from some nits that should be easily addressable on the commit.

LGTM aside from a couple of nits. Feel free to address those on the commit.

LGTM. I have no way of verifying this but no reason to doubt it.

LGTM. The remaining comments are just nits that you can address on the commit. Thanks for your patience through this review and for addressing all the comments.

LGTM. Thanks for cleaning it up. I was mainly concerned with making sure this is an omission rather than something that was done on purpose to work around some bug. But it does indeed appear to be something we accidentally added.

I think this would be immensely more readable if you use more descriptive names for variables. It is very hard to get this bit manipulation right in ones head so I really think you should try your best to make this as simple to follow as possible:

1. You use one mask in on line 4463 and then a different one on 4476. Please don't do this. Stick with a consistent example.
2. Rename RotateRightClearLeft to something like RightJustifyRangeAndClear as it appears that is what the function is doing.
3. Get rid of all the expressions involving ME/MB - especially things like <imm> +/- MB/ME as they are very difficult to reason about. For readability, favour defining temporary values just so they would have a name. For example: MB+63-ME is kind of meaningless to a reader. But if you do something like unsigned FirstBitSetWhenRightJustified = MB + 63 - ME; that is now much easier to follow. I realize that we are creating single-use temporaries this way, but I really think it is worth it for readability.

In D76240#1930775, @grimar wrote:

In D76240#1927115, @MaskRay wrote:

For lld, I think --threads={0,1,2,...} is better than the current --(no-)threads.

BTW, can't testing with --no-threads hide possible LLDs concurency bugs?
We probably do not want to test LLD with --(no-)threads in most cases because it is not a real world use case of LLD.

While I agree that the more general issue of scaling LLVM's (and particularly LLD's) parallelism is something we should consider tackling, I still think there is value in proceeding with something along the lines of this patch in the interim.
For users that have very parallel builds, there are mitigations available:

• Presumably, most builds do not link such a large number of binaries that this is a serious problem
• Even for builds that do link a large number of binaries, the cmake/ninja build system does provide the ability to scale down the number of parallel link jobs as someone mentioned in the comments above

LGTM aside from a nit from me and a couple from clang-format. I'm sure you can address those on the commit.

I think it would be useful to add a binary test case using either llvm-objdump or readelf if that is available. Just to test the desired encoding.

Approving this even though there is a problem with the logic in SelectAddressPCRel() as I trust that you will fix the obvious bug when committing this.

My comments are quite minor so feel free to address them on the commit. LGTM.

In D76240#1925329, @MaskRay wrote:

The -Wl,--no-threads logic seems test machine specific. Shouldn't a site-wise configuration file added somewhere on the test machine to adapt the behavior of lit substitutions? I don't know how to do that, though. lit.site.cfg.py files are generated in the build directory. They are not used for customization.

In D76240#1925275, @cryptoad wrote:

Shouldn't -DLLVM_PARALLEL_LINK_JOBS=1 be used to avoid the described condition?

LGTM aside from some minor nits. Feel free to address those on the commit.

In D76240#1925419, @efriedma wrote:

"overwhelmed" by what, exactly? I mean, I guess there's fundamentally some overhead if we end up with N^2 active threads (where N is the number of cores), but threads don't have that much memory overhead on their own; it shouldn't be high in absolute terms unless your test machine somehow has hundreds of cores. And I'm not sure what else could cause an issue; I guess if the kernel scheduler decided to do something weird, context switches might hurt performance?

Also, what OS are you using?

$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.6 LTS"

I really think we should add significantly more testing to this patch. We should have test cases to cover the following (we can also just add run lines with -mcpu=future to existing tests):

• Calls from functions that use the TOC (i.e. globals, etc.)
• Calls from functions that don't use the TOC
• Indirect calls
• Tail calls (i.e. that they are disabled)
• Handling of X2 in leaf functions (since this has changed AFAICT)

This is a curious situation. How did we end up having multiple definitions of instruction aliases?

The changes that are needed are fairly minor but I would like to take another look. I promise to get to it immediately when you have the updated patch posted.

LGTM. Sorry I missed this for so long. Thank you for fixing it.

I am not sure I am actually following what it is you are requesting in terms of testing. Perhaps I need to describe the problems I am solving more thoroughly.

Removed egregious use of auto and added a vfabi atrribute test case.

In D74944#1887086, @fpetrogalli wrote:

Hi @nemanjai ,

thank you for updating the code.

Sorry for being picky, I think you should add another test. Your code need to work also for the vector-function-avi-variant attribute and not just for the -vector-library= option.

Kind regards,

Francesco

Updated to use the info from VFDatabase. Thanks @fpetrogalli!

It would appear that this commit makes the following case trip an assert. Once the obvious change is made for the BranchInst cast to dyn_cast, it trips the assert because there is a load that uses the global.

You're right, -O0 shouldn't generate FMA. I'm preparing to revert this now -- just verifying the build.

Perhaps this should be
off with no optimization
on with -O1/-O2/-O3/-Os/-Oz
fast with fast math

Thanks for the comments. I'll update the patch prior to committing.

Fixed up the nits in the test cases and added a test for soft float. Also, moved the pattern fragments to the target independent td file for consistency.

Thanks for the comments, I'll address them and upload an update shortly.

My good intentions of updating and uploading the test case were once again thwarted by git in the previous update. Actually included the test case on this one.

The unit test failure is due to an unrelated commit to libc++. I'm not sure why it is showing up here, but presumably when I upload any new updates and the bot re-runs the tests, that should go away.

Updated the test case.

@lei @stefanp Could you please re-review as this would be good to fix since it fixes a reported bug? I would like to get it committed and backported to V10.0.

LGTM.

LGTM.

Align the descriptions of the suboptions.

LGTM. Thanks for addressing the comments.

Forgot to select request changes.

A few more comments need to be addressed for this to proceed.

The shift amount for one of the patterns was wrong for little endian.

This introduces a dependency for https://reviews.llvm.org/D72569.

LGTM.

Aside from a couple of minor nits, LGTM.

If I understand this correctly, this just evaluates the query for lock free atomics at compile time if the size is larger than the maximum possible size for an atomic on the target. If that's the case, this looks fine to me. But of course, some of the other target maintainers might feel otherwise.
The incompatibility with GCC might be an issue for some, but I don't expect this to be an issue at least on PPC.

Adding Florian, Daniel and Craig as reviewers since they were involved in the most recent change to this area of table gen.