[AArch64] Check opcode before trying to extract register from operand

[tellenbach]

Summary

When matching FNEG patterns for the MachineCombiner we need to check for
opcodes first, before trying to extract a register from an operand.
Otherwise handling of instructions with non-register operands causes the
compiler to crash.

[Gerolf]

LGTM

[jroelofs]

Suggest using llvm/utils/update_llc_test_checks.py to write the CHECKs, so they're easier to update later.

Otherwise LGTM.

[MattDevereau]

The change looks good to me but the test needs tightening up.

inline-asm-empty-template.ll Is too generic of a file name, and it's not actually a template. This should be specifically referencing fneg.

llvm/test/CodeGen/AArch64/inline-asm-empty-template.ll

6 ↗

(On Diff #552180)

This test should be renamed to something that reflects what this is actually testing instead of just @a. @emit_fneg_with_non_register_operand or something along those lines. At a glance you can't really tell why this test exists out of context with the changes.

17 ↗

(On Diff #552180)

This line should be explicitly checked, or else the test is too flimsy. I can replace this line with %fneg = fadd double %1, %1 which doesn't test your change at all and this test still passes.

; CHECK: fneg d1, d0

[dmgreen]

It may be simpler for the test to be an mir test if it testing the MachineCombiner. It should be less susceptible to bitrot if any of the proceeding optimizations before it change.

[tellenbach]

Thanks, I've added a mir test that checks for fneg and gave it the name Matt suggested.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

static bool getFNEGPatterns(MachineInstr &Root,
                             SmallVectorImpl<MachineCombinerPattern> &Patterns) {
   unsigned Opc = Root.getOpcode();
   MachineBasicBlock &MBB = *Root.getParent();
   MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
 
   auto Match = [&](unsigned Opcode, MachineCombinerPattern Pattern) -> bool {
     MachineOperand &MO = Root.getOperand(1);
     MachineInstr *MI = MRI.getUniqueVRegDef(MO.getReg());
-    if (MI != nullptr && MRI.hasOneNonDBGUse(MI->getOperand(0).getReg()) &&
-        (MI->getOpcode() == Opcode) &&
+    if (MI != nullptr && (MI->getOpcode() == Opcode) &&
+        MRI.hasOneNonDBGUse(MI->getOperand(0).getReg()) &&
         Root.getFlag(MachineInstr::MIFlag::FmContract) &&
         Root.getFlag(MachineInstr::MIFlag::FmNsz) &&
         MI->getFlag(MachineInstr::MIFlag::FmContract) &&
         MI->getFlag(MachineInstr::MIFlag::FmNsz)) {
       Patterns.push_back(Pattern);
       return true;
     }
     return false;

llvm/test/CodeGen/AArch64/emit_fneg_with_non_register_operand.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
+# RUN: llc -mtriple aarch64 -run-pass=machine-combiner -o - %s | FileCheck %s
+--- |
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64"
+
+  @c = global double 0.000000e+00, align 8
+
+  define void @emit_fneg_with_non_register_operand(double %c) {
+  entry:
+    %0 = load double, ptr @c, align 8
+    %1 = tail call double asm sideeffect "", "=w,0"(double %0)
+    %2 = load double, ptr @c, align 8
+    %3 = tail call double asm sideeffect "", "=w,0"(double %2)
+    %fneg = fneg double %1
+    %cmp = fcmp oeq double %3, %fneg
+    br i1 %cmp, label %if.then, label %if.end
+
+  if.then:                                          ; preds = %entry
+    tail call void @b(double noundef %1)
+    ret void
+
+  if.end:                                           ; preds = %entry
+    ret void
+  }
+
+  declare void @b(double noundef)
+
+...
+---
+name:            emit_fneg_with_non_register_operand
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
+callsEHReturn:   false
+callsUnwindInit: false
+hasEHCatchret:   false
+hasEHScopes:     false
+hasEHFunclets:   false
+isOutlined:      false
+debugInstrRef:   false
+failsVerification: false
+tracksDebugUserValues: false
+registers:
+  - { id: 0, class: fpr64, preferred-register: '' }
+  - { id: 1, class: fpr64, preferred-register: '' }
+  - { id: 2, class: fpr64, preferred-register: '' }
+  - { id: 3, class: fpr64, preferred-register: '' }
+  - { id: 4, class: fpr64, preferred-register: '' }
+  - { id: 5, class: fpr64, preferred-register: '' }
+  - { id: 6, class: gpr64common, preferred-register: '' }
+  - { id: 7, class: fpr64, preferred-register: '' }
+liveins:         []
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    1
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  functionContext: ''
+  maxCallFrameSize: 0
+  cvBytesOfCalleeSavedRegisters: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  hasTailCall:     true
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      []
+stack:           []
+entry_values:    []
+callSites:       []
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  ; CHECK-LABEL: name: emit_fneg_with_non_register_operand
+  ; CHECK: bb.0.entry:
+  ; CHECK-NEXT:   successors: %bb.1(0x50000000), %bb.2(0x30000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   [[LOADgot:%[0-9]+]]:gpr64common = LOADgot target-flags(aarch64-got) @c
+  ; CHECK-NEXT:   [[LDRDui:%[0-9]+]]:fpr64 = LDRDui [[LOADgot]], 0 :: (dereferenceable load (s64) from @c)
+  ; CHECK-NEXT:   INLINEASM &"", 1 /* sideeffect attdialect */, 2359306 /* regdef:FPR64 */, def %2, 2147483657 /* reguse tiedto:$0 */, [[LDRDui]](tied-def 3)
+  ; CHECK-NEXT:   [[COPY:%[0-9]+]]:fpr64 = COPY %2
+  ; CHECK-NEXT:   [[LDRDui1:%[0-9]+]]:fpr64 = LDRDui [[LOADgot]], 0 :: (dereferenceable load (s64) from @c)
+  ; CHECK-NEXT:   INLINEASM &"", 1 /* sideeffect attdialect */, 2359306 /* regdef:FPR64 */, def %4, 2147483657 /* reguse tiedto:$0 */, [[LDRDui1]](tied-def 3)
+  ; CHECK-NEXT:   [[FNEGDr:%[0-9]+]]:fpr64 = FNEGDr %2
+  ; CHECK-NEXT:   nofpexcept FCMPDrr %4, killed [[FNEGDr]], implicit-def $nzcv, implicit $fpcr
+  ; CHECK-NEXT:   Bcc 1, %bb.2, implicit $nzcv
+  ; CHECK-NEXT:   B %bb.1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1.if.then:
+  ; CHECK-NEXT:   $d0 = COPY [[COPY]]
+  ; CHECK-NEXT:   TCRETURNdi @b, 0, csr_aarch64_aapcs, implicit $sp, implicit $d0
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2.if.end:
+  ; CHECK-NEXT:   RET_ReallyLR
+  bb.0.entry:
+    successors: %bb.1(0x50000000), %bb.2(0x30000000)
+
+    %6:gpr64common = LOADgot target-flags(aarch64-got) @c
+    %3:fpr64 = LDRDui %6, 0 :: (dereferenceable load (s64) from @c)
+    INLINEASM &"", 1 /* sideeffect attdialect */, 2359306 /* regdef:FPR64 */, def %2, 2147483657 /* reguse tiedto:$0 */, %3(tied-def 3)
+    %0:fpr64 = COPY %2
+    %5:fpr64 = LDRDui %6, 0 :: (dereferenceable load (s64) from @c)
+    INLINEASM &"", 1 /* sideeffect attdialect */, 2359306 /* regdef:FPR64 */, def %4, 2147483657 /* reguse tiedto:$0 */, %5(tied-def 3)
+    %7:fpr64 = FNEGDr %2
+    nofpexcept FCMPDrr %4, killed %7, implicit-def $nzcv, implicit $fpcr
+    Bcc 1, %bb.2, implicit $nzcv
+    B %bb.1
+
+  bb.1.if.then:
+    $d0 = COPY %0
+    TCRETURNdi @b, 0, csr_aarch64_aapcs, implicit $sp, implicit $d0
+
+  bb.2.if.end:
+    RET_ReallyLR
+
+...