I've just found there's no commit message. In general it could be helpful to include one or two short sentences describing the actual change, e.g. "Optimize bitfield extractions retaining bit positions with bstrpick + slli", because "optimize foo" otherwise doesn't carry useful information about the actual improvements.

Good catch, thanks!

In D147305#4237973, @benshi001 wrote:

In D147305#4237944, @SixWeining wrote:

Thanks for the improvements.
So this is only for the case 2 mentioned by @xen0n, right? Seems that the test for 81 is missing.

Will case 1 and case 3 be handled later?

Thanks for your comments.

1. The missing case 81 is added.
2. I will implement case 1 and case 3 later in another patch. BTW: Total amount of these two cases is small, and they even have redundant values with case 2, so how about implementing them without foreach, just standalone Pat one by one ?

In D147305#4236226, @benshi001 wrote:

In D147305#4236071, @xen0n wrote:

This is good strength reduction overall, thanks for the insight!

Though maybe the exact patterns can be automatically generated with nested loops, for reduced mental burden to prove correctness? I've sketched something and it seems doable:

>>> alsl = lambda j, k, imm: (j << imm) + k
>>> a = [alsl(alsl(1, 1, i), alsl(1, 1, i), j) for j in range(1,5) for i in range(1,5)]
>>> a
[9, 15, 27, 51, 15, 25, 45, 85, 27, 45, 81, 153, 51, 85, 153, 289]
>>> list(sorted(set(a)))
[9, 15, 25, 27, 45, 51, 81, 85, 153, 289]
>>> b = [alsl(alsl(1, 1, i), 1, j) for j in range(1,5) for i in range(1,5)]
>>> b
[7, 11, 19, 35, 13, 21, 37, 69, 25, 41, 73, 137, 49, 81, 145, 273]
>>> ab = set(a).union(set(b))
>>> len(ab)
24
>>> list(sorted(set(a).intersection(set(b))))
[25, 81]

>>> c = [alsl(1, alsl(1, 1, i), j) for j in range(1,5) for i in range(1,5)]
>>> c
[5, 7, 11, 19, 7, 9, 13, 21, 11, 13, 17, 25, 19, 21, 25, 33]
>>> list(sorted(set(c)))
[5, 7, 9, 11, 13, 17, 19, 21, 25, 33]

>>> set(c).difference(ab)
{33, 5, 17}
>>> all = set(c).union(ab).difference({3, 5, 9, 17})
>>> len(all)
24
>>> list(sorted(all))
[7, 11, 13, 15, 19, 21, 25, 27, 33, 35, 37, 41, 45, 49, 51, 69, 73, 81, 85, 137, 145, 153, 273, 289]

So basically, we can strength-reduce a total of 24 different constant-multiplications with two alsl's:

• case 1: alsl T, X, X, i; alsl Y, T, T, j: 15, 25, 27, 45, 51, 81, 85, 153, 289
• case 2: alsl T, X, X, i; alsl Y, T, X, j: 7, 11, 13, 19, 21, 25, 35, 37, 41, 49, 69, 73, 81, 137, 145, 273
• case 3: alsl T, X, X, i; alsl Y, X, T, j: 7, 11, 13, 19, 21, 25, 33

Problem is that there are some overlaps between the 3 possible combinations, and some inside case 1 and 3. If we could somehow avoid producing conflicting rules then probably leveraging TableGen's loop and computation abilities would produce code that's easier to maintain. Otherwise, simplifying the code with some macros could also be beneficial.

Thanks for your suggestion! Using foreach really makes the code more clear!

rebase

This is good strength reduction overall, thanks for the insight!

Nit: this is preparatory change adding baseline test cases for D147222, so the title may be tweaked a little bit, or you may add some commit message, to not suggest such tests were totally absent otherwise.

Typo in title: "immediates".

Document the change in Clang release note.

Rebase and change to lower the op in LoongArchAsmPrinter so as to make people's lives easier when they come to rebase D141785 on top of this.

update other affected tests

In D138135#4117010, @MQ-mengqing wrote:

Fix the pattern for bstrpick.w on LA64.

In D143880#4121913, @xen0n wrote:

@xry111: BTW your GCC implementation of this pattern looks for *arithmetic* right shifts; do you mean logical instead? Because obviously the higher bits would be full of ones after shifting if rj happen to have its MSB set. The sign extension shall happen *after* the pick.

Self note: the pattern may need more love, because the signext i32 cases appear wrong on LA64.

@xry111: BTW your GCC implementation of this pattern looks for *arithmetic* right shifts; do you mean logical instead? Because obviously the higher bits would be full of ones after shifting if rj happen to have its MSB set. The sign extension shall happen *after* the pick.

Rebase & split tests out & implement matching of paired addu16i.d + addi patterns

Some nice improvements it seems!

Seems good; you could rewrite these tests in opaque pointers afterwards as well.

rebase

rebase and update testcase

LGTM with the nit addressed, thanks!

Better than parsing /proc/cpuinfo which I was nearly going to do, only hindered by my day job. Thanks!

Looks good, thanks!

rebase

add test cases for PCALA relocs, fix getLoongArchPageOffset's adjustment to the higher half as uncovered by the tests

fix a switch arm and tweak comments

amend comments

fix PCALA_HI20 behavior in case of PCALA_LO12 on JIRL, and cleanups

In D138135#4071143, @SixWeining wrote:

I just verified the latest change, lld still fail to link llvm and report the error:

ld.lld: error: relocation R_LARCH_PCALA_HI20 cannot be used against symbol '__cxa_atexit'; recompile with -fPIC                                                                              
>>> defined in /usr/lib64/libc.so.6
>>> referenced by atexit.c:46 (../stdlib/atexit.c:46)
>>>               atexit.oS:(atexit) in archive /usr/lib64/libc_nonshared.a
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

In D142278#4071050, @xry111 wrote:

By the way, using PCALA_LO12 like this means lld will need to peek at the instruction and see if it's a jirl to handle the PCALA_LO12 relocation properly. I guess we'll need to investigate this issue a little. I hope it won't cause too much trouble.

In D142278#4071001, @xry111 wrote:

LGTM though I really dislike depending on some undocumented behavior of PCALA_LO12.

I only realized it's the Chinese New Year's Eve after finishing this and seeing your comments. /facepalm

Add a couple of tests, implement the R_LARCH_PCALA_LO12 on JIRL kludge

The architecture-specific details LGTM, thanks! A couple of nits below.

Aren't all the register indexes less human-friendly to most? ;-)

Hi, as I've recently tried to bootstrap Gentoo on LoongArch with LLVM toolchain, I've run into multiple build failures that needed to be worked around; given how many high-profile projects are impacted, is it prudent to temporarily downgrade this to a warning? mold chose to emit a warning for such cases, for example.

In D138135#4051965, @brad wrote:

With the backend enabled, it would be good to work on this further.

Thanks for the patch!

BTW do we have a way of fixing the bug while preserving the alias?

As the community guy who has helped the port a little (by writing some of the codegen myself, porting LLD, putting all of these into distro-building practice and helping troubleshooting things in general), I can confirm the port indeed works on most open-source projects I've checked. Even Linux is close to being buildable. I also experimented with Gentoo stage-building with a recent LLVM snapshot (with my D138135 applied), and bootstrapping is almost done with all problems so far unrelated to the target code.

In D140685#4017381, @xen0n wrote:

MOVGR2FCSR modifies the value of the software writable field
corresponding to the FCSR (floating-point control and status
register) fcsr according to the value of the lower 32 bits of
the GR (general purpose register) rj.

The description of movgr2fcsr is incorrect, it implies GPR[rj]is read, but in fact it's only the ui5 immediate in rj slot, i.e. FCSR[rj]. I didn't look very closely but the test case changes seem good.

MOVGR2FCSR modifies the value of the software writable field
corresponding to the FCSR (floating-point control and status
register) fcsr according to the value of the lower 32 bits of
the GR (general purpose register) rj.

revert the broken TLS IE offset change

This is cleaner than previous revisions (which contained code model related changes) which is good. I've slightly adjusted the patch summary to remove the trivial bits and now inaccurate descriptions.

Seems something's broken with the change. I'll have to take care of this patch later (hopefully around early January) due to day job.

• rebased
• fixed TLS IE relocs (need an offset of 2 GOT entries) according to @MQ-mengqing's comment
• added a few test cases; more in the works
• minor refactoring

This change makes sense on LoongArch, and should cause no behavioral change on 4KiB-page platforms like X86, but more testing on other architectures (e.g. AArch64 systems with 16KiB or 64KiB pages) would be welcome.

LGTM, thanks!

This is nice for being able to know if the pipeline is being affected by a change. Thanks for the addition.

This change is actually platform-agnostic. While it looks good to me (minus one nit) I'd like to leave the approval to someone else more familiar with this code.

LGTM, thanks very much for the contribution!

Linux only requires __cpucfg among the ones you just added. Please amend the patch summary so it's more accurate.

Thank you very much, this now looks good from the LoongArch side.

Favor touching nothing besides the lit directive as suggested by @lebedev.ri.

rebase

This should be good enough (reminiscent of CSR ops) but let's wait for other reviewers.