Address the comments. And all the case changes disappear with the patch https://reviews.llvm.org/D72706 landed. So, I didn't see the negative impact from this patch any more. And from the design, it is doing the right thing, without increasing the compiling time.

Only alias the safe one.

Ping ...

Hmm, cause the sanity failure.

/home/qshanz/work/commit/build/bin/llc -mtriple=thumbv7-none-linux-gnueabi -debug -o /dev/null < /home/qshanz/work/commit/llvm/test/CodeGen/ARM/gpr-paired-spill-thumbinst.ll 2>&1 | /home/qshanz/work/commit/build/bin/FileCheck /home/qshanz/work/commit/llvm/test/CodeGen/ARM/gpr-paired-spill-thumbinst.ll
/home/qshanz/work/commit/llvm/test/CodeGen/ARM/gpr-paired-spill-thumbinst.ll:19:10: error: CHECK: expected string not found in input
; CHECK: t2STRDi8
         ^
<stdin>:1:1: note: scanning from here
Args: /home/qshanz/work/commit/build/bin/llc -mtriple=thumbv7-none-linux-gnueabi -debug -o /dev/null
^
<stdin>:21:25: note: possible intended match here
.. opcode 39 is aliased to 38

In D72031#1807588, @fhahn wrote:

This adds new dependencies, hence I think it would be good to gather code size/perf numbers with this change for some impacted targets (e.g. AArch64) to be reasonably sure that there are no unexpected knock-on effects.

Use the isNegatibleForFree/getNegatibleExpression mechanism.

This is really a good suggestion. I will go with that mechanism, though it seems that, it didn't respect the TLI.isFNegFree() and UnsafeFPMath which didn't make sense. (When it is FNEG opcode, we just return 2 to indicate that it is profitable. What if some target is free for FNEG operations ...)

Format the patch.

Ping ...

I like this patch, as it really takes me a while to understand what it is before your patch.

LGTM. But please hold on some days in case someone else has other comments.

In D71983#1799470, @hfinkel wrote:

Do you expect any effect at all from doing this? These are all scheduling barriers?

Yes, it is just moving the code into a new function to avoid too much code in the switch case. And it is definition more clear if moving the condition into small functions. As there are several different conditions that could produce the same instruction with different arguments. I will do it as follows.

bool tryAndWithMask(...) {

I get the reason about 0x8000 now.

#ha(value) Denotes the high adjusted value: bits 16 - 63 of the indicated value, compensating
for #lo() being treated as a signed number. That is:
#ha(x) = (x + 0x8000) >> 16

The TOC region commonly includes data items within the .got, .toc, .sdata, and .sbss sections. In the medium
code model, they can be addressed with 32-bit signed offsets from the TOC pointer register. The TOC pointer
register typically points to the beginning of the .got section + 0x8000, which permits a 2 GB TOC with the
medium and large code models.

In D72069#1800924, @fhahn wrote:

In D72069#1800869, @steven.zhang wrote:

If you want to pass it with parameter, why not change the signature of releaseNode directly instead of adding a wrapper. We need to make decision about template arguments vs function parameter :)

+1. I think just passing InPQueue would keep things simpler and it should get inlined anyways ;)

In D58378#1800862, @MaskRay wrote:

b.c:(.text+0x8): relocation truncated to fit: R_PPC64_TOC16_HA against symbol `b' defined in COMMON section in /tmp/b-9d97be.o+7ffffff8

The r_addend is 0x7ffffff8, a value close to 2**31.

The distance between the TOC entry and the variable address cannot be too far. More accurately, -0x80008000 <= address - .TOC. + r_addend < 0x7fff8000

GNU ld correctly reports a relocation overflow. lld currently does not check R_PPC64_TOC16_HA overflow.

% powerpc64le-linux-gnu-ld -pie b.o main.o
powerpc64le-linux-gnu-ld: warning: cannot find entry symbol _start; defaulting to 0000000000000230
powerpc64le-linux-gnu-ld: b.o: in function `foo':
b.c:(.text+0x8): relocation truncated to fit: R_PPC64_TOC16_HA against symbol `b' defined in COMMON section in b.o+7ffffff8

The largest address a pair of HA/L can materialize is something like:

addis 3, 2, 32767  # adding 1 will overflow to -32768
lfd 1, 32767(3)

If you want to pass it with parameter, why not change the signature of releaseNode directly instead of adding a wrapper. We need to make decision about template arguments vs function parameter :)

In D65506#1800633, @DoktorC wrote:

Hi,

bad luck, got a build failure.
Link to the report: http://lab [.] llvm [.] org:8011/builders/clang-ppc64be-linux-lnt/builds/33875

Build Reason: scheduler
Build Source Stamp: [branch master] f9f78cf6ac73d9148be9b626f418bf6770e512f6
Blamelist: Lorenzo Casalino <lorenzo.casalino93@gmail.com>

BUILD FAILED: failed test-suite

Still need to investigate why it is 27 bit limit.

Address comments about the test case.

Rebase the patch due to the AArch64 commit 65651f197a2c which try to legalize sext_inreg for vector type for SVE. We need to specify it explicitly instead of relying on the default value. I will hold on this patch for several days in case someone has comments on the change of "SVE" part.

I realize that most reviewers were on vacation during this patch was reviewed and committed. It is a bit too haste to commit this patch. Please add the comments here if have any concern. Sorry about this.

I realize that most reviewers were on vacation during this patch was reviewed and committed. It is a bit too haste to commit this patch. Please add the comments here if have any concern. Sorry about this.

The RLWINMo should also be removed,

I think, you don't need to post phabricator review for adding tests, except that, you are not confident on the way how it is wrote. LGTM.

LGTM as it makes sense. But please hold on for some days in case someone else have other concern. And I suggest you commit follow up patches to set this bit for other branch instructions.

Address comments.

You need to revisit the place that use the isPredicable bit of the MI, which might cause functionality change. i.e.

bool TargetInstrInfo::isUnpredicatedTerminator(const MachineInstr &MI) const {
  if (!MI.isTerminator()) return false;

Ping ...

Rebase the patch. Testing with spec and get about ~4k instructions reduce.

Remove the 16-bit limit as we will have benefit even it is within 16bit, as rlwinm won't define the CR register while ANDIo did.

LGTM with some minor nit.

We need another patch to fix the dead instruction removal for record form I think.

In D71833#1794987, @shchenz wrote:

Thanks for finding the bug in the testcases. I have modified them in NFC patch https://reviews.llvm.org/rG79b3325be0b016fdc1a2c55bce65ec9f1e5f4eb6

Could you please help to be more specific about the invalid case in the description?

Below c code gives a valid result:

#include <stdio.h>

#define rlwinm( output, input, sh, mb, me ) \
   __asm__( "rlwinm %0, %1, %2, %3, %4" \
          : "=r"(output) \
          : "r"(input), "i"(sh), "i"(mb), "i"(me) \
          : )

int main()
{
   unsigned long y,z,t ;

   int i = 0;

   for (unsigned x = 0; x < 4294967295; x++)  {
     rlwinm( y, x, 27, 30, 10) ;
     rlwinm( z, y, 19, 0, 12);
     rlwinm(t, x, 14, 11, 12);
     if (z != t) {
       printf("0x%016lX -> 0x%016lX, 0x%016lX\n", x, y, z);
       printf("0x%016lX -> 0x%016lX\n", x, t);
       i = 100;
       break;
    }
   }
   if (i == 0)
     printf("valid\n");
   else
     printf("invalid\n");
   return 0;
}

So I guess

%2:gprc = RLWINM %1:gprc, 27, 30, 10
%3:gprc = RLWINM %2:gprc, 19, 0, 12

should always be equal to

%3:gprc = RLWINM %1, 14, 11, 12

for type i32 and i64 also?

Transfer to Zheng to continue working on this.

LGTM.