[PATCH] D41775: [ARM] Add codegen for SMMULR, SMMLAR and SMMLSR

[Sam Parker via Phabricator via llvm-commits]

samparker added a comment.

Hi Andre,

Probably best to break this into two patches, one for the fix and one for the feature change.

cheers,
sam



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9865
 
 static SDValue AddCombineTo64bitMLAL(SDNode *AddeNode,
                                      TargetLowering::DAGCombinerInfo &DCI,
----------------
Now we're handing sub as well as add, I suggest renaming the argument.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10047
+  if (AddcNode->getOpcode() != ARMISD::ADDC &&
+      AddcNode->getOpcode() != ARMISD::SUBC)
     return SDValue();
----------------
The name of the function suggests it should break return if it can't find UMAAL, so I think we should just search for that here. I'm pretty sure I wrote this, but it seems misleading that the search for this one instruction then can generate all the other DSP muls. Therefore I feel that it would be a good idea for readability to break this search up. 


================
Comment at: lib/Target/ARM/ARMISelLowering.h:206
       SMLSLDX,      // Signed multiply subtract long dual exchange
+      MULHS_AR,     // Signed multiply long, round and add
+      MULHS_SR,     // Signed multiply long, subtract and round
----------------
Any reason for not maintaining the naming convention?


================
Comment at: test/CodeGen/ARM/dsp-mlal.ll:1
+; RUN: llc -mtriple=thumbv7m -mattr=+dsp %s -o - | FileCheck %s
+; RUN: llc -mtriple=armv7a %s -o - | FileCheck %s
----------------
For all of these, its best to test that you've got the instruction operands correct.


https://reviews.llvm.org/D41775