[PATCH] D41766: [MachineCombiner] Add check for optimal pattern order.

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added a comment.
Herald added subscribers: StephenFan, pengfei.
Herald added projects: LLVM, All.

Hi @fhahn 
We are testing AArch64 builds with EXPENSIVE_CHECK=ON and SPEC 2017 and llvm test-suite are failing with the assert in this patch.

  assert(CurrentLatencyDiff <= PrevLatencyDiff &&
             "Current pattern is better than previous pattern.");

In addition to the above benchmarks, there are a couple of other internal benchmarks failing too. Here is a small reproducer we got with creduce.

  long a, b, c, d;
  void f() {
    while (!0) {
      long e = c * b - d * a;
      if (e < 7)
        break;
    }
  }



  Command-line: clang  -O2 reduced.c

I have a high-level question for this patch. I understand that the verify function aims to verify patterns to be present in certain order but insertion is not guaranteed to be in the same order.

Does it make sense to have this verification in the first place?

FWIW, the following patch would fix the issue but it is not a scalable approach:

  host:~/llvm-GRCO-105/build_release$ git diff ../llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  index d49b82290ff3..ed1ccb7b7484 100644
  --- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  +++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  @@ -5143,8 +5143,8 @@ static bool getMaddPatterns(MachineInstr &Root,
       setFound(AArch64::MADDWrrr, 2, AArch64::WZR, MCP::MULSUBW_OP2);
       break;
     case AArch64::SUBXrr:
  -    setFound(AArch64::MADDXrrr, 1, AArch64::XZR, MCP::MULSUBX_OP1);
       setFound(AArch64::MADDXrrr, 2, AArch64::XZR, MCP::MULSUBX_OP2);
  +    setFound(AArch64::MADDXrrr, 1, AArch64::XZR, MCP::MULSUBX_OP1);
       break;
     case AArch64::ADDWri:
       setFound(AArch64::MADDWrrr, 1, AArch64::WZR, MCP::MULADDWI_OP1); 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D41766/new/

https://reviews.llvm.org/D41766