Page MenuHomePhabricator

[ARM] Add codegen for SMMULR, SMMLAR and SMMLSR
ClosedPublic

Authored by avieira on Jan 5 2018, 9:39 AM.

Details

Summary

This patch teaches the Arm back-end to generate the SMMULR, SMMLAR and SMMLSR instructions from equivalent IR patterns.

I forbid generation of these if '-arm-use-mulops=false' is passed to the command-line since t2SMMLAR and t2SMMLSR have UseMulOps as a predicate.
Adding bob.wilson to make sure this is intended behavior.

This patch also fixes an issue where SMMLS was being generated for Armv7-M, where it should only be generated for Armv7E-M (aka Armv7-M with DSP extensions).

Diff Detail

Repository
rL LLVM

Event Timeline

avieira created this revision.Jan 5 2018, 9:39 AM

Hi Andre,

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

cheers,
sam

lib/Target/ARM/ARMISelLowering.cpp
9865 ↗(On Diff #128765)

Now we're handing sub as well as add, I suggest renaming the argument.

10047 ↗(On Diff #128765)

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.

lib/Target/ARM/ARMISelLowering.h
206 ↗(On Diff #128765)

Any reason for not maintaining the naming convention?

test/CodeGen/ARM/dsp-mlal.ll
1 ↗(On Diff #128765)

For all of these, its best to test that you've got the instruction operands correct.

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.

lib/Target/ARM/ARMISelLowering.cpp
9865 ↗(On Diff #128765)

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..

10047 ↗(On Diff #128765)

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

lib/Target/ARM/ARMISelLowering.h
206 ↗(On Diff #128765)

Hmmm, sure so use the instruction names they may map to? SMMLSR and SMMLAR?

test/CodeGen/ARM/dsp-mlal.ll
1 ↗(On Diff #128765)

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...

avieira updated this revision to Diff 129051.Jan 9 2018, 2:28 AM

Split the patch, see fix in: https://reviews.llvm.org/D41855
Also applied other suggested changes.

samparker accepted this revision.Jan 9 2018, 5:36 AM

Great, LGTM, cheers!

This revision is now accepted and ready to land.Jan 9 2018, 5:36 AM
rogfer01 added inline comments.Jan 10 2018, 12:58 AM
lib/Target/ARM/ARMISelLowering.cpp
1341 ↗(On Diff #129051)

Minor nit: this is misaligned.

avieira added inline comments.Jan 12 2018, 1:25 AM
lib/Target/ARM/ARMISelLowering.cpp
1341 ↗(On Diff #129051)

Thx, will fix this in the commit.

This revision was automatically updated to reflect the committed changes.