That's certainly completely wrong, and, indeed, bolsters my point that MIPS16 object file emission can't possibly have worked before.

LGTM.

Nit: the summary should have the line--"This resolves PR/54991" to aid in bug tracking.

Thanks for the review, @atanasyan . I went with changing the regexes back to the style in the existing file.

Thanks for the review.

Thanks for the review, I'll try to get this committed in the next few days.

NFC: Remove some extraneous test lines and a small grammar correction

Thanks for the review, I've updated the bug number in the commit message.

LGTM.

LGTM with the nit addressed.

LGTM.

Thank you for the patch.

Ok, I'll see about committing this tomorrow.

LGTM. Do you have commit access?

These MIPS changes look OK to me.

Should that be a follow-up patch, or included in this one?

Hi @impiaaa, thanks for looking this.

Two nits inlined. Otherwise this patch looks OK, eyeballing the supplied test.

Some post commit remarks.

LGTM.

In D66228#1632790, @Petar.Avramovic wrote:

...
What difference does SYM_32/SYM_64 make, all selected instructions use only 16 bits of something that is resolved later?

In D66228#1635859, @atanasyan wrote:

Thanks for review. Could you clarify some points in your comments?

In D66228#1632060, @sdardis wrote:

The correct approach I believe is to fix the SYM_32 bug, then provide the patterns for cases of where intermediate/end nodes such as MipsHi / MipsLo can appear without an add appearing as their parent node.

1. If we do not change the getAddrNonPICSym64 function, at some point (before lowering) we anyway(?) get the following chain of commands (add (shl (%hi(sym), 16), %lo(%sym)). How can we lower it to a correct set of instructions, if we do not have a pattern with the shl? Please correct we if I'm wrong.

I took a second look, and I believe this patch is the incorrect solution. The bug actually lies in the implementation of PredicateControl and SYM_32/SYM_64, @Petar.Avramovic nearly spotted it.

This looks OK to me, I'd like to take a hard look at what DAGCombiner is doing here.

LGTM.

I've raised some points inline, I think (a) is most relevant to this patch but shouldn't stop it going forward but should be addressed quickly, (b) and (c) could be noted as TODO:s.

In D48069#1479260, @beanz wrote:

Is there a specific version range of GCC that this miscompile occurs with?

I think this is a viable approach for this issue. Based on the dumps from -debug I can see that the backend should only be selecting even register for FGR32.

My initial comments are that this looks ok, but I would like a longer look at it.

Good catch. LGTM.

One nit inline, but otherwise LGTM.

Some minor nits inlined, but otherwise the patch is more or less there.

Two comments, and some more inlined. Check the formatting of the commit message
body, it should be 80 cols max. Otherwise the log messages can look strange due
to wrapping (I'm using git).

Sorry I didn't spot this earlier, but in future please ensure 'llvm-commits' is one of the subscribers when creating a review request for LLVM. If you add it after creating a review request, manually add it and write something in the comments field to trigger Phabricator into sending an email or abandon the review request and re-open it with the relevant -commits list as an initial subscriber.

LGTM apart from some minor nits. Please address them before committing.

In D57718#1393615, @Carrot wrote:

In D57718#1393504, @jsji wrote:

Any testcases that can show the problem and test the fix? Thanks.

It can only be triggered by a very large (>32KB) function body because the range of conditional branch is +/- 32KB.

In our case, the large function body is caused by aggressive thinlto guided inlining.

So no small test case can demonstrate the problem :(

I can't find rationale behind the MIPS discrepancy in the original commit. I can add the if branch back if you tell me why...

LGTM. Just remember to upload the patches with context.

Ping.

Ping.

Address comment.

Ping.

Looking at this again and with trunk, I'm seeing two failing tests with a small change inlined:

Added tests, and corrected the definition of the rdhwr instruction.