[PATCH] D100218: Fix for "Bug 49146 - Crash with MIPS16 multiply"

[Jesse DeGuire via Phabricator via llvm-commits]

jdeguire marked 3 inline comments as done.
jdeguire added a comment.

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.



================
Comment at: clang/test/CodeGen/mips16-mult.c:1
+// REQUIRES: mips-registered-target
+// RUN: %clang --target=mipsel-unknown-linux -mips16 -c -o %t %s
----------------
atanasyan wrote:
> 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.
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.


================
Comment at: llvm/lib/Target/Mips/MipsISelLowering.cpp:208
   case MipsISD::Multu:             return "MipsISD::Multu";
+  case MipsISD::Mult16:
+    return "MipsISD::Mult16";
----------------
atanasyan wrote:
> It's better to keep formattings the same as the code around.
Understood, reverted.


================
Comment at: llvm/lib/Target/Mips/MipsISelLowering.h:57
 
-    enum NodeType : unsigned {
-      // Start the numbering from where ISD NodeType finishes.
----------------
atanasyan wrote:
> Please revert this too big change unrelated to this patch.
Reverted.


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

https://reviews.llvm.org/D100218