Fix for "Bug 49146 - Crash with MIPS16 multiply"

[jdeguire]

Summary

This is a potential fix for a bug I reported here (Bug 49146) in which a basic integer multiply instruction will crash Clang/LLVM when building for MIPS16. The test code I used is in the bug report. I manually verified the patch by examining the resulting disassembly from "llvm-objdump" to see the resulting "MULT->MFLO" instructions. I'm not sure if this patch should have been submitted to that bug report or here, so please let know me know if I'm in the wrong place.

(Note that there might be another minor bug in which a 3-bit "zero" field in the MIPS16 MFLO/MFHI is not set to zero, but I can look at that more closely and submit another patch later.)

The issue appeared to be that an "ISD::MUL" operation was being translated to a pseudo-instruction called "Mult16RxRyRz", but that pseudo-instruction was never translated to the real instruction sequence. Other operations that require the MIPS HI/LO registers are manually translated in the MIPS target-specific code and so this patch removes that pseudo-instruction and follows how the other HI/LO instructions are handled.

I'm not familiar with the LLVM review process and so any help with regards to documentation, providing tests, etc. would be appreciated.

[RKSimon]

Sorry, but I don't know enough about MIPS16 to help with this.

I'd recommend adding some tests though!

[jdeguire]

Updated the patch to add a very simple test. This test fails pre-patch and passes after the patch. This test does not check for correctness of the generated code, only that Clang does not crash.

[jdeguire]

This applies "git format-clang" to the patch. I didn't do this originally because, as you can see, "git clang-format" makes changes that do not match the style of the surrounding code and so I was not sure what the proper course of action was. I can always either resubmit the previous patch or make another patch (maybe for a separate review) that re-formats the rest of the code.

[atanasyan]

clang/test/CodeGen/mips16-mult.c

2

1. You need to create a test case in the llvm/test/CodeGen/Mips folder. Sometimes LLVM build without Clang.
2. Test case should check result of code generation. Not only the fact that the program does not crash.

llvm/lib/Target/Mips/MipsISelLowering.cpp

208

It's better to keep formattings the same as the code around.

llvm/lib/Target/Mips/MipsISelLowering.h

57

Please revert this too big change unrelated to this patch.

[jdeguire]

As requested, this patch removes the formatting changes that "git clang-format" had made to the previous patch. This also updates the test in clang/test/CodeGen to verify that assembly for a MULT, MFLO sequence is emitted. An equivalent test was generated from clang -S -emit-llvm and cleaned up to be added to llvm/test/CodeGen/Mips.

While I was able to update the test to verify that assembly was generated, I may need guidance as to verifying direct code generation (ie. calling Clang without the "-S" or "-emit-llvm" options). My original plan was to generate an object file, give that to llvm-objdump, and pipe that output to FileCheck, but I cannot get llvm-objdump to emit decoded MIPS16 assembly. It emits raw bytes and instructions are listed as "<unknown>" when using --triple=mipsel-linux-gnu --mattr=mips16. I was able to manually decode the output to verify that the raw bytes corresponded to the correct MULT, MFLO sequence, but I am not sure how I can reliably do that with FileCheck. Should I try to have FileCheck search for the raw sequence of bytes (which in this case is 78 ea 52 ea)?

[jdeguire]

Apologies for the very, very late responses to your comments. Yes, it really did take me 3 weeks to figure out that the "Submit" button is at the very bottom.

I would still appreciate any guidance on verifying direct code generation (or if that's even normally done as part of these tests). Thank you.

clang/test/CodeGen/mips16-mult.c

2

Mostly done, anyway. As stated my in most recent patch update, I was able to complete (1) but (2) I was able to complete for assembly output but not for direct machine code generation due to issues I'm having with llvm-objdump.

llvm/lib/Target/Mips/MipsISelLowering.cpp

208

Understood, reverted.

llvm/lib/Target/Mips/MipsISelLowering.h

57

Reverted.

clang/test/CodeGen/mips16-mult.c

+// REQUIRES: mips-registered-target
+// RUN: %clang --target=mipsel-unknown-linux -mips16 -c -o %t %s
+ 
+// Check for BugZilla bug 49146 "Crash with MIPS16 Multiply".
+// The bug does not manifest when using the "-S" or "-emit-llvm" options.
+
+unsigned do_mult(unsigned n, unsigned m)
+{
+    return n * m;
+}

llvm/lib/Target/Mips/Mips16ISelDAGToDAG.cpp

bool Mips16DAGToDAGISel::trySelect(SDNode *Node) {
   ///
   EVT NodeTy = Node->getValueType(0);
   unsigned MultOpc;
 
   switch (Opcode) {
   default:
     break;
 
+  case ISD::MUL: {
+    MultOpc = Mips::MultRxRy16;
+    auto LoHi = selectMULT(Node, MultOpc, DL, NodeTy, true, false);
+    ReplaceNode(Node, LoHi.first);
+    return true;
+  }
+
   /// Mul with two results
   case ISD::SMUL_LOHI:
   case ISD::UMUL_LOHI: {
     MultOpc = (Opcode == ISD::UMUL_LOHI ? Mips::MultuRxRy16 : Mips::MultRxRy16);
     std::pair<SDNode *, SDNode *> LoHi =
         selectMULT(Node, MultOpc, DL, NodeTy, true, true);
     if (!SDValue(Node, 0).use_empty())
       ReplaceUses(SDValue(Node, 0), SDValue(LoHi.first, 0));

llvm/lib/Target/Mips/Mips16InstrInfo.td

 // I8_MOV32R instruction format (used only by MOV32R instruction)
 //
 
 class FI8_MOV32R16_ins<string asmstr, InstrItinClass itin>:
   FI8_MOV32R16<(outs GPR32:$r32), (ins CPU16Regs:$rz),
                !strconcat(asmstr,  "\t$r32, $rz"), [], itin>;
 
 //
-// This are pseudo formats for multiply
-// This first one can be changed to non-pseudo now.
-//
-// MULT
-//
-class FMULT16_ins<string asmstr, InstrItinClass itin> :
-  MipsPseudo16<(outs), (ins CPU16Regs:$rx, CPU16Regs:$ry),
-               !strconcat(asmstr, "\t$rx, $ry"), []>;
-
-//
-// MULT-LO
-//
-class FMULT16_LO_ins<string asmstr, InstrItinClass itin> :
-  MipsPseudo16<(outs CPU16Regs:$rz), (ins CPU16Regs:$rx, CPU16Regs:$ry),
-               !strconcat(asmstr, "\t$rx, $ry\n\tmflo\t$rz"), []> {
-  let isCodeGenOnly=1;
-}
-
-//
 // RR-type instruction format
 //
 
 class FRR16_ins<bits<5> f, string asmstr, InstrItinClass itin> :
   FRR16<f, (outs CPU16Regs:$rx), (ins CPU16Regs:$ry),
         !strconcat(asmstr, "\t$rx, $ry"), [], itin> {
 }
 

 
 //
 // maybe refactor but need a $zero as a dummy first parameter
 //
 class FRR16_div_ins<bits<5> f, string asmstr, InstrItinClass itin> :
   FRR16<f, (outs ), (ins CPU16Regs:$rx, CPU16Regs:$ry),
         !strconcat(asmstr, "\t$$zero, $rx, $ry"), [], itin> ;
 
+class FRR16_mult_ins<bits<5> f, string asmstr, InstrItinClass itin> :
+  FRR16<f, (outs ), (ins CPU16Regs:$rx, CPU16Regs:$ry),
+        !strconcat(asmstr, "\t$rx, $ry"), [], itin> ;
+
 class FUnaryRR16_ins<bits<5> f, string asmstr, InstrItinClass itin> :
   FRR16<f, (outs CPU16Regs:$rx), (ins CPU16Regs:$ry),
         !strconcat(asmstr, "\t$rx, $ry"), [], itin> ;
 
 
 class FRR16_M_ins<bits<5> f, string asmstr,
                   InstrItinClass itin> :
   FRR16<f, (outs CPU16Regs:$rx), (ins),

 //
 // Format: DIVU rx, ry MIPS16e
 // Purpose: Divide Unsigned Word
 // To divide 32-bit unsigned integers.
 //
 def DivuRxRy16: FRR16_div_ins<0b11011, "divu", IIM16Alu> {
   let Defs = [HI0, LO0];
 }
+
 //
 // Format: JAL target MIPS16e
 // Purpose: Jump and Link
 // To execute a procedure call within the current 256 MB-aligned
 // region and preserve the current ISA.
 //
 
 def Jal16 : FJAL16_ins<0b0, "jal", IIM16Alu> {

 //
 def Mflo16: FRR16_M_ins<0b10010, "mflo", IIM16Alu> {
   let Uses = [LO0];
   let hasSideEffects = 0;
   let isMoveReg = 0;
 }
 
 //
-// Pseudo Instruction for mult
-//
-def MultRxRy16:  FMULT16_ins<"mult",  IIM16Alu> {
-  let isCommutable = 1;
-  let hasSideEffects = 0;
-  let Defs = [HI0, LO0];
-}
-
-def MultuRxRy16: FMULT16_ins<"multu", IIM16Alu> {
-  let isCommutable = 1;
-  let hasSideEffects = 0;
-  let Defs = [HI0, LO0];
-}
-
-//
 // Format: MULT rx, ry MIPS16e
 // Purpose: Multiply Word
 // To multiply 32-bit signed integers.
 //
-def MultRxRyRz16: FMULT16_LO_ins<"mult", IIM16Alu> {
-  let isCommutable = 1;
-  let hasSideEffects = 0;
+def MultRxRy16: FRR16_mult_ins<0b11000, "mult", IIM16Alu> {
   let Defs = [HI0, LO0];
 }
 
 //
 // Format: MULTU rx, ry MIPS16e
 // Purpose: Multiply Unsigned Word
 // To multiply 32-bit unsigned integers.
 //
-def MultuRxRyRz16: FMULT16_LO_ins<"multu", IIM16Alu> {
-  let isCommutable = 1;
-  let hasSideEffects = 0;
+def MultuRxRy16: FRR16_mult_ins<0b11001, "multu", IIM16Alu> {
   let Defs = [HI0, LO0];
 }
 
+
 //
 // Format: NEG rx, ry MIPS16e
 // Purpose: Negate
 // To negate an integer value.
 //
 def NegRxRy16: FUnaryRR16_ins<0b11101, "neg", IIM16Alu>;
 
 //

 def: ArithLogicU_pat<ineg, NegRxRy16>;
 
 class ArithLogic16_pat<SDNode OpNode, Instruction I> :
   Mips16Pat<(OpNode CPU16Regs:$l, CPU16Regs:$r),
             (I CPU16Regs:$l, CPU16Regs:$r)>;
 
 def: ArithLogic16_pat<add, AdduRxRyRz16>;
 def: ArithLogic16_pat<and, AndRxRxRy16>;
-def: ArithLogic16_pat<mul, MultRxRyRz16>;
 def: ArithLogic16_pat<or, OrRxRxRy16>;
 def: ArithLogic16_pat<sub, SubuRxRyRz16>;
 def: ArithLogic16_pat<xor, XorRxRxRy16>;
 
 // Arithmetic and logical instructions with 2 register operands.
 
 class ArithLogicI16_pat<SDNode OpNode, PatFrag imm_type, Instruction I> :
   Mips16Pat<(OpNode CPU16Regs:$in, imm_type:$imm),

 
 //
 // MipsDivRemU
 //
 def: Mips16Pat
   <(MipsDivRemU16 CPU16Regs:$rx, CPU16Regs:$ry),
    (DivuRxRy16 CPU16Regs:$rx, CPU16Regs:$ry)>;
 
+//
+// MipsMult
+//
+def: Mips16Pat
+  <(MipsMult16 CPU16Regs:$rx, CPU16Regs:$ry),
+   (MultRxRy16 CPU16Regs:$rx, CPU16Regs:$ry)>;
+
+//
+// MipsMultu
+//
+def: Mips16Pat
+  <(MipsMultu16 CPU16Regs:$rx, CPU16Regs:$ry),
+   (MultuRxRy16 CPU16Regs:$rx, CPU16Regs:$ry)>;
+
 //  signed a,b
 //  x = (a>=b)?x:y
 //
 //  if !(a < b) x = y
 //
 def : Mips16Pat<(select (i32 (setge CPU16Regs:$a, CPU16Regs:$b)),
                  CPU16Regs:$x, CPU16Regs:$y),
                 (SelTBteqZSlt CPU16Regs:$x, CPU16Regs:$y,

llvm/lib/Target/Mips/MipsISelLowering.h

 class MipsFunctionInfo;
 class MipsSubtarget;
 class MipsTargetMachine;
 class TargetLibraryInfo;
 class TargetRegisterClass;
 
   namespace MipsISD {
 
     enum NodeType : unsigned {
       // Start the numbering from where ISD NodeType finishes.
       FIRST_NUMBER = ISD::BUILTIN_OP_END,
 
       // Jump and link (call)
       JmpLink,
 
       // Tail call
       TailCall,
 
       // Get the Highest (63-48) 16 bits from a 64-bit immediate
       Highest,
 
       // Get the Higher (47-32) 16 bits from a 64-bit immediate
       Higher,
 
       // Get the High 16 bits from a 32/64-bit immediate
       // No relation with Mips Hi register
       Hi,
 
       // Get the Lower 16 bits from a 32/64-bit immediate
       // No relation with Mips Lo register
       Lo,
 
       // Get the High 16 bits from a 32 bit immediate for accessing the GOT.
       GotHi,
 
       // Get the High 16 bits from a 32-bit immediate for accessing TLS.
       TlsHi,
 
       // Handle gp_rel (small data/bss sections) relocation.
       GPRel,
 
       // Thread Pointer
       ThreadPointer,
 
       // Vector Floating Point Multiply and Subtract
       FMS,
 
enum NodeType : unsigned {
       MFLO,
 
       // Node used to insert integers to accumulator.
       MTLOHI,
 
       // Mult nodes.
       Mult,
       Multu,
+      Mult16,
+      Multu16,
 
       // MAdd/Sub nodes
       MAdd,
       MAddu,
       MSub,
       MSubu,
 
       // DivRem(u)

llvm/lib/Target/Mips/MipsISelLowering.cpp

const char *MipsTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case MipsISD::CMovFP_T:          return "MipsISD::CMovFP_T";
   case MipsISD::CMovFP_F:          return "MipsISD::CMovFP_F";
   case MipsISD::TruncIntFP:        return "MipsISD::TruncIntFP";
   case MipsISD::MFHI:              return "MipsISD::MFHI";
   case MipsISD::MFLO:              return "MipsISD::MFLO";
   case MipsISD::MTLOHI:            return "MipsISD::MTLOHI";
   case MipsISD::Mult:              return "MipsISD::Mult";
   case MipsISD::Multu:             return "MipsISD::Multu";
+  case MipsISD::Mult16:            return "MipsISD::Mult16";
+  case MipsISD::Multu16:           return "MipsISD::Multu16";
   case MipsISD::MAdd:              return "MipsISD::MAdd";
   case MipsISD::MAddu:             return "MipsISD::MAddu";
   case MipsISD::MSub:              return "MipsISD::MSub";
   case MipsISD::MSubu:             return "MipsISD::MSubu";
   case MipsISD::DivRem:            return "MipsISD::DivRem";
   case MipsISD::DivRemU:           return "MipsISD::DivRemU";
   case MipsISD::DivRem16:          return "MipsISD::DivRem16";
   case MipsISD::DivRemU16:         return "MipsISD::DivRemU16";

llvm/lib/Target/Mips/MipsInstrInfo.td

 def SDT_MFLOHI : SDTypeProfile<1, 1, [SDTCisInt<0>, SDTCisVT<1, untyped>]>;
 def SDT_MTLOHI : SDTypeProfile<1, 2, [SDTCisVT<0, untyped>,
                                       SDTCisInt<1>, SDTCisSameAs<1, 2>]>;
 def SDT_MipsMultDiv : SDTypeProfile<1, 2, [SDTCisVT<0, untyped>, SDTCisInt<1>,
                                     SDTCisSameAs<1, 2>]>;
 def SDT_MipsMAddMSub : SDTypeProfile<1, 3,
                                      [SDTCisVT<0, untyped>, SDTCisSameAs<0, 3>,
                                       SDTCisVT<1, i32>, SDTCisSameAs<1, 2>]>;
-def SDT_MipsDivRem16 : SDTypeProfile<0, 2, [SDTCisInt<0>, SDTCisSameAs<0, 1>]>;
+def SDT_MipsMultDiv16 : SDTypeProfile<0, 2, [SDTCisInt<0>, SDTCisSameAs<0, 1>]>;
 
 def SDT_MipsThreadPointer : SDTypeProfile<1, 0, [SDTCisPtrTy<0>]>;
 
 def SDT_Sync             : SDTypeProfile<0, 1, [SDTCisVT<0, i32>]>;
 
 def SDT_Ext : SDTypeProfile<1, 3, [SDTCisInt<0>, SDTCisSameAs<0, 1>,
                                    SDTCisVT<2, i32>, SDTCisSameAs<2, 3>]>;
 def SDT_Ins : SDTypeProfile<1, 4, [SDTCisInt<0>, SDTCisSameAs<0, 1>,

 // Nodes used to extract LO/HI registers.
 def MipsMFHI : SDNode<"MipsISD::MFHI", SDT_MFLOHI>;
 def MipsMFLO : SDNode<"MipsISD::MFLO", SDT_MFLOHI>;
 
 // Node used to insert 32-bit integers to LOHI register pair.
 def MipsMTLOHI : SDNode<"MipsISD::MTLOHI", SDT_MTLOHI>;
 
 // Mult nodes.
-def MipsMult  : SDNode<"MipsISD::Mult", SDT_MipsMultDiv>;
-def MipsMultu : SDNode<"MipsISD::Multu", SDT_MipsMultDiv>;
+def MipsMult    : SDNode<"MipsISD::Mult", SDT_MipsMultDiv>;
+def MipsMultu   : SDNode<"MipsISD::Multu", SDT_MipsMultDiv>;
+def MipsMult16  : SDNode<"MipsISD::Mult16", SDT_MipsMultDiv16>;
+def MipsMultu16 : SDNode<"MipsISD::Multu16", SDT_MipsMultDiv16>;
 
 // MAdd*/MSub* nodes
 def MipsMAdd  : SDNode<"MipsISD::MAdd", SDT_MipsMAddMSub>;
 def MipsMAddu : SDNode<"MipsISD::MAddu", SDT_MipsMAddMSub>;
 def MipsMSub  : SDNode<"MipsISD::MSub", SDT_MipsMAddMSub>;
 def MipsMSubu : SDNode<"MipsISD::MSubu", SDT_MipsMAddMSub>;
 
 // DivRem(u) nodes
 def MipsDivRem    : SDNode<"MipsISD::DivRem", SDT_MipsMultDiv>;
 def MipsDivRemU   : SDNode<"MipsISD::DivRemU", SDT_MipsMultDiv>;
-def MipsDivRem16  : SDNode<"MipsISD::DivRem16", SDT_MipsDivRem16,
+def MipsDivRem16  : SDNode<"MipsISD::DivRem16", SDT_MipsMultDiv16,
                            [SDNPOutGlue]>;
-def MipsDivRemU16 : SDNode<"MipsISD::DivRemU16", SDT_MipsDivRem16,
+def MipsDivRemU16 : SDNode<"MipsISD::DivRemU16", SDT_MipsMultDiv16,
                            [SDNPOutGlue]>;
 
 // Target constant nodes that are not part of any isel patterns and remain
 // unchanged can cause instructions with illegal operands to be emitted.
 // Wrapper node patterns give the instruction selector a chance to replace
 // target constant nodes that would otherwise remain unchanged with ADDiu
 // nodes. Without these wrapper node patterns, the following conditional move
 // instruction is emitted when function cmov2 in test/CodeGen/Mips/cmov.ll is

llvm/lib/Target/Mips/MipsScheduleGeneric.td

 
 // divu, udiv
 def : InstRW<[GenericWriteDIV], (instrs MOD, MODU, DIV, DIVU)>;
 
 
 // MIPS16e
 // =======
 
-def : InstRW<[GenericWriteHILO], (instrs MultRxRy16, MultuRxRy16,
-                                  MultRxRyRz16, MultuRxRyRz16)>;
+def : InstRW<[GenericWriteHILO], (instrs MultRxRy16, MultuRxRy16)>;
 
 def : InstRW<[GenericWriteDIV], (instrs DivRxRy16)>;
 
 def : InstRW<[GenericWriteDIVU], (instrs DivuRxRy16)>;
 
 // microMIPS
 // =========