[M68k] Add instruction selection support for zext with PCD addressing

[ids1024]

Summary

Instruction selection was failing when trying to zero extend a value loaded from a PC-relative address. This adds support for zero extension using the "program counter indirect with displacement" addressing mode. It also adds a test with code that was previously failing to compile.

This fixes a compile error in Rust's libcore.

[glaubitz]

Thanks for your patch! I just gave it a try but I'm still running into this error when trying to build libcore:

In function: _ZN4core3num7dec2flt6lemire13compute_float17h5485431827d3a879E
error: could not compile `core`
LLVM ERROR: Cannot select: t13: i32 = mul t12, t36
  t12: i32,ch = CopyFromReg t0, Register:i32 %27
    t11: i32 = Register %27
  t36: i32,ch = load<(load (s8) from %ir.70, !alias.scope !40, !noalias !45), zext from i8> t0, t2, undef:i32
    t2: i32,ch = CopyFromReg t0, Register:i32 %30
      t1: i32 = Register %30
    t4: i32 = undef
In function: _ZN83_$LT$memchr..memmem..FindIter$u20$as$u20$core..iter..traits..iterator..Iterator$GT$4next17h404867d129058870E
error: could not compile `memchr`
rustc: /data/home/glaubitz/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:10477: void llvm::SelectionDAG::ReplaceAllUsesWith(llvm::SDNode*, llvm::SDNode*): Assertion `(!From->hasAnyUseOfValue(i) || From->getValueType(i) == To->getValueType(i)) && "Cannot use this version of ReplaceAllUsesWith!"' failed.
rustc exited with signal: 6 (SIGABRT) (core dumped)
error: could not compile `rustc-demangle`

Do we need to change something else?

[ids1024]

In D149034#4291901, @glaubitz wrote:

Thanks for your patch! I just gave it a try but I'm still running into this error when trying to build libcore:

In particular, this patch fixes compilation of parse_long_mantissa. That error message doesn't seem to involve PC-relative addressing so it wouldn't be impacted by this patch. It may be unrelated or a different issue with zero extension.

I think there are other issues related to this. Even if zero-extension doesn't support PCD addressing, it presumably should be able to load the value and zero extend it with more instructions.

[ids1024]

Actually, after too much debugging, it looks like that error is an issue with mul when targeting the 68020. You can work around that by changing setOperationAction(ISD::MUL, MVT::i32, Legal); to setOperationAction(ISD::MUL, MVT::i32, LibCall);, but after that I still have a build error LLVM ERROR: Cannot select: t42: ch = AtomicStore<(store unordered (s16) into %ir.30)> t40, t17, t38, src/mem/mod.rs:77:13.

So some more instruction selection fixes are needed for mul and AtomicStore.

[glaubitz]

In D149034#4301169, @ids1024 wrote:

Actually, after too much debugging, it looks like that error is an issue with mul when targeting the 68020. You can work around that by changing setOperationAction(ISD::MUL, MVT::i32, Legal); to setOperationAction(ISD::MUL, MVT::i32, LibCall);, but after that I still have a build error LLVM ERROR: Cannot select: t42: ch = AtomicStore<(store unordered (s16) into %ir.30)> t40, t17, t38, src/mem/mod.rs:77:13.

So some more instruction selection fixes are needed for mul and AtomicStore.

Thanks a lot for debugging this. I had suspected MUL to be one of the problems, but I still lack some understanding on how LLVM IR works.

Maybe Sheng and Min can comment on this change and the other necessary fixes.

[myhsu]

LGTM Thanks!
Though could you update the patch description so that it's more descriptive? You can take a look at the commit message policy.

In D149034#4301169, @ids1024 wrote:

Actually, after too much debugging, it looks like that error is an issue with mul when targeting the 68020. You can work around that by changing setOperationAction(ISD::MUL, MVT::i32, Legal); to setOperationAction(ISD::MUL, MVT::i32, LibCall);, but after that I still have a build error LLVM ERROR: Cannot select: t42: ch = AtomicStore<(store unordered (s16) into %ir.30)> t40, t17, t38, src/mem/mod.rs:77:13.

So some more instruction selection fixes are needed for mul and AtomicStore.

I wouldn't lower 32-bit MUL to libcall when targeting >68020 since it already has native instructions for that size. For the AtomicStore issue it should be an easy fix: lower it to libcall same as AtomicLoad.

[ids1024]

I wouldn't lower 32-bit MUL to libcall when targeting >68020 since it already has native instructions for that size.

Yeah, definitely a workaround rather than a correct fix. But the fact it works with libcall and not legal suggests something is needed in instruction selection to properly generate the multiply instruction here. If I understand correctly.

[ids1024]

Hm, that compile error seems to go away if I set ATOMIC_STORE to use Expand, but not LibCall? I'm not sure exactly how these are converted to library calls or "expanded". With that and a couple other hacks, Rust's libcore seems to compile.

[glaubitz]

@myhsu Could you land this change? I think @ids1024 doesn't have commit access.

llvm/lib/Target/M68k/M68kExpandPseudo.cpp

return TII->ExpandMOVSZX_RM(MIB, false, TII->get(M68k::MOV8df), MVT::i16,
                                 MVT::i8);
   case M68k::MOVZXd32f8:
     return TII->ExpandMOVSZX_RM(MIB, false, TII->get(M68k::MOV8df), MVT::i32,
                                 MVT::i8);
   case M68k::MOVZXd32f16:
     return TII->ExpandMOVSZX_RM(MIB, false, TII->get(M68k::MOV16rf), MVT::i32,
                                 MVT::i16);
 
+  case M68k::MOVZXd16q8:
+    return TII->ExpandMOVSZX_RM(MIB, false, TII->get(M68k::MOV8dq), MVT::i16,
+                                MVT::i8);
+  case M68k::MOVZXd32q8:
+    return TII->ExpandMOVSZX_RM(MIB, false, TII->get(M68k::MOV8dq), MVT::i32,
+                                MVT::i8);
+  case M68k::MOVZXd32q16:
+    return TII->ExpandMOVSZX_RM(MIB, false, TII->get(M68k::MOV16dq), MVT::i32,
+                                MVT::i16);
+
   case M68k::MOV8cd:
     return TII->ExpandCCR(MIB, /*IsToCCR=*/true);
   case M68k::MOV8dc:
     return TII->ExpandCCR(MIB, /*IsToCCR=*/false);
 
   case M68k::MOVM8jm_P:
     return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32jm), /*IsRM=*/false);
   case M68k::MOVM16jm_P:

llvm/lib/Target/M68k/M68kInstrData.td

let mayLoad = 1 in {
       def MOV#EXT#Xd16p8   : MxPseudoMove_RM<MxType16d,  MxType8.POp>;
       def MOV#EXT#Xd32p8   : MxPseudoMove_RM<MxType32d,  MxType8.POp>;
       def MOV#EXT#Xd32p16  : MxPseudoMove_RM<MxType32d, MxType16.POp>;
 
       def MOV#EXT#Xd16f8   : MxPseudoMove_RM<MxType16d,  MxType8.FOp>;
       def MOV#EXT#Xd32f8   : MxPseudoMove_RM<MxType32d,  MxType8.FOp>;
       def MOV#EXT#Xd32f16  : MxPseudoMove_RM<MxType32d, MxType16.FOp>;
 
+      def MOV#EXT#Xd16q8   : MxPseudoMove_RM<MxType16d,  MxType8.QOp>;
+      def MOV#EXT#Xd32q8   : MxPseudoMove_RM<MxType32d,  MxType8.QOp>;
+      def MOV#EXT#Xd32q16  : MxPseudoMove_RM<MxType32d,  MxType16.QOp>;
+
     }
   }
 }
 
 /// This group of instructions is similar to the group above but DOES NOT do
 /// any value extension, they just load a smaller register into the lower part
 /// of another register if operands' real registers are different or does
 /// nothing if they are the same.

 def: Pat<(i16 (zext i8:$src)),
           (EXTRACT_SUBREG (MOVZXd32d8 MxDRD8:$src), MxSubRegIndex16Lo)>;
 def: Pat<(MxZExtLoadi16i8 MxCP_ARI:$src),
           (EXTRACT_SUBREG (MOVZXd32j8 MxARI8:$src), MxSubRegIndex16Lo)>;
 def: Pat<(MxZExtLoadi16i8 MxCP_ARID:$src),
           (EXTRACT_SUBREG (MOVZXd32p8 MxARID8:$src), MxSubRegIndex16Lo)>;
 def: Pat<(MxZExtLoadi16i8 MxCP_ARII:$src),
           (EXTRACT_SUBREG (MOVZXd32f8 MxARII8:$src), MxSubRegIndex16Lo)>;
+def: Pat<(MxZExtLoadi16i8 MxCP_PCD :$src), (MOVZXd16q8 MxPCD8 :$src)>;
 
 // i32 <- zext i8
 def: Pat<(i32 (zext i8:$src)), (MOVZXd32d8 MxDRD8:$src)>;
 def: Pat<(MxZExtLoadi32i8 MxCP_ARI :$src), (MOVZXd32j8 MxARI8 :$src)>;
 def: Pat<(MxZExtLoadi32i8 MxCP_ARID:$src), (MOVZXd32p8 MxARID8:$src)>;
 def: Pat<(MxZExtLoadi32i8 MxCP_ARII:$src), (MOVZXd32f8 MxARII8:$src)>;
+def: Pat<(MxZExtLoadi32i8 MxCP_PCD :$src), (MOVZXd32q8 MxPCD8 :$src)>;
 
 // i32 <- zext i16
 def: Pat<(i32 (zext i16:$src)), (MOVZXd32d16 MxDRD16:$src)>;
 def: Pat<(MxZExtLoadi32i16 MxCP_ARI :$src), (MOVZXd32j16 MxARI16 :$src)>;
 def: Pat<(MxZExtLoadi32i16 MxCP_ARID:$src), (MOVZXd32p16 MxARID16:$src)>;
 def: Pat<(MxZExtLoadi32i16 MxCP_ARII:$src), (MOVZXd32f16 MxARII16:$src)>;
+def: Pat<(MxZExtLoadi32i16 MxCP_PCD :$src), (MOVZXd32q16 MxPCD16 :$src)>;
 
 // i16 <- anyext i8
 def: Pat<(i16 (anyext i8:$src)),
           (EXTRACT_SUBREG (MOVZXd32d8 MxDRD8:$src), MxSubRegIndex16Lo)>;
 def: Pat<(MxExtLoadi16i8 MxCP_ARI:$src),
           (EXTRACT_SUBREG (MOVZXd32j8 MxARI8:$src), MxSubRegIndex16Lo)>;
 def: Pat<(MxExtLoadi16i8 MxCP_ARID:$src),
           (EXTRACT_SUBREG (MOVZXd32p8 MxARID8:$src), MxSubRegIndex16Lo)>;

llvm/test/CodeGen/M68k/load-extend.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc < %s -mtriple=m68k-linux -verify-machineinstrs | FileCheck %s
+
+@0 = external constant <{ [32 x i8] }>
+
+define i32 @"test_zext_pcd_i8_to_i32"() {
+; CHECK-LABEL: test_zext_pcd_i8_to_i32:
+; CHECK:         .cfi_startproc
+; CHECK-NEXT:  ; %bb.0:
+; CHECK-NEXT:    move.b (__unnamed_1+16,%pc), %d0
+; CHECK-NEXT:    and.l #255, %d0
+; CHECK-NEXT:    rts
+  %p = getelementptr inbounds i8, ptr @0, i32 16
+  %val = load i8, ptr %p
+  %val2 = zext i8 %val to i32
+  ret i32 %val2
+}
+
+define i16 @"test_zext_pcd_i8_to_i16"() {
+; CHECK-LABEL: test_zext_pcd_i8_to_i16:
+; CHECK:         .cfi_startproc
+; CHECK-NEXT:  ; %bb.0:
+; CHECK-NEXT:    move.b (__unnamed_1+16,%pc), %d0
+; CHECK-NEXT:    and.w #255, %d0
+; CHECK-NEXT:    rts
+  %p = getelementptr inbounds i8, ptr @0, i32 16
+  %val = load i8, ptr %p
+  %val2 = zext i8 %val to i16
+  ret i16 %val2
+}
+
+define i32 @"test_zext_pcd_i16_to_i32"() {
+; CHECK-LABEL: test_zext_pcd_i16_to_i32:
+; CHECK:         .cfi_startproc
+; CHECK-NEXT:  ; %bb.0:
+; CHECK-NEXT:    move.w (__unnamed_1+16,%pc), %d0
+; CHECK-NEXT:    and.l #65535, %d0
+; CHECK-NEXT:    rts
+  %p = getelementptr inbounds i16, ptr @0, i32 8
+  %val = load i16, ptr %p
+  %val2 = zext i16 %val to i32
+  ret i32 %val2
+}