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

[Andre Vieira via Phabricator via llvm-commits]

avieira added a comment.

Thanks for the comments. The split is definitely a good idea, also I have some questions/feedback on the comments. Ill post a new patch here for the code generation and a separate one for the fix.



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9865
 
 static SDValue AddCombineTo64bitMLAL(SDNode *AddeNode,
                                      TargetLowering::DAGCombinerInfo &DCI,
----------------
samparker wrote:
> Now we're handing sub as well as add, I suggest renaming the argument.
Hmm so also with respect to the comment below I did think about splitting this, but most of the checks up until the generation of the ARMISD nodes for SMMLSR/SMMLAR/SMMULR are the same so I would basically be duplicating code.

Just changing Adde also wouldnt make sense so Ill be changing all "*Add*" variable names to mean both so AddeSubeNode, AddeSubeOp(0,1), AddcSubcNode, LowAddSub and so on..


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10047
+  if (AddcNode->getOpcode() != ARMISD::ADDC &&
+      AddcNode->getOpcode() != ARMISD::SUBC)
     return SDValue();
----------------
samparker wrote:
> 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. 
I am no longer entirely sure I even need this code here... This function should only be reachable by the ADDE node and SMMLSR comes in as a SUBE/SUBC pair. So it flows from PerformAddeSubeCombine to AddCombineTo64bitMLAL


================
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
----------------
samparker wrote:
> Any reason for not maintaining the naming convention?
Hmmm, sure so use the instruction names they may map to? SMMLSR and SMMLAR?


================
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
----------------
samparker wrote:
> For all of these, its best to test that you've got the instruction operands correct.
Sure, but Ill accept two cases per instruction where the multiplication operands are swapped, since these are equivalent. I doubt codegen would change to flip them but yeh...


https://reviews.llvm.org/D41775