This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add macro fusion for AES instructions.
ClosedPublic

Authored by fhahn on Jun 13 2017, 6:00 AM.

Details

Summary

This patch adds a macro fusion using CodeGen/MacroFusion.cpp to pair AES
instructions back to back and adds FeatureFuseAES to enable the feature.

Diff Detail

Event Timeline

fhahn created this revision.Jun 13 2017, 6:00 AM
javed.absar edited edge metadata.Jun 13 2017, 1:20 PM

Thanks Florian for this. Some comments below -

lib/Target/ARM/ARM.td
103–104

nitpick - Formatting seems different from rest of defs in this file.
... : SubtargetFeature<"fuse-aes", "HasFuseAES", "true",

lib/Target/ARM/ARMMacroFusion.cpp
42

Should this check be upfront so it comes out immediately?

In fact, to TargetMachine.cpp, for efficiency ?
const ARMSubtarget &ST = C->MF->getSubtarget<ARMSubtarget>();
if (ST.hasFuseAES())

DAG->addMutation(createARMMacroFusionDAGMutation());
lib/Target/ARM/CMakeLists.txt
52

why add twice?

fhahn updated this revision to Diff 102507.Jun 14 2017, 2:32 AM
fhahn marked an inline comment as done.

Addressed feedback, thanks @javed.absar

lib/Target/ARM/ARMMacroFusion.cpp
42

Yes! I'll add a check before adding the DAG mutation. I think it makes sense to keep the check here as well though, in case we add fusion for other instructions as well.

evandro added inline comments.Jun 14 2017, 9:27 AM
lib/Target/ARM/ARMTargetMachine.cpp
399

Granted that fuse-aes is currently the only fusion, but I wonder if it´d make sense to create a more encompassing hasFusion() method that would combine all and any instruction fusion.

javed.absar added inline comments.Jun 14 2017, 9:41 AM
lib/Target/ARM/ARMTargetMachine.cpp
399

In light of that, should 'shouldScheduleAdjacent' be renamed 'shouldScheduleAdjacentAES' ?
I understand you have an equivalent patch for AArch64 of this, so any ask here would need parallel changes there and depending on how things converge on https://reviews.llvm.org/D34144

fhahn updated this revision to Diff 103039.Jun 19 2017, 6:59 AM

rebased and added hasFusion() function.

lib/Target/ARM/ARMTargetMachine.cpp
399

I've added a hasFusion() function. I think it might be worth adding a similar function on AArch64, which I will do once this change is committed.

I committed the generic MacroFusion pass yesterday (D34144) and this patch is already updated to work with the committed version.

LGTM but I will wait a day before accepting to allow time for others to comment too, just in case.
Thanks for this.

javed.absar accepted this revision.Jun 22 2017, 1:15 AM
This revision is now accepted and ready to land.Jun 22 2017, 1:15 AM
fhahn added a comment.Jun 22 2017, 2:39 AM

Thanks Javed!

fhahn closed this revision.Jun 22 2017, 2:40 AM
evandro added inline comments.Jul 3 2017, 9:40 AM
lib/Target/ARM/ARMTargetMachine.cpp
399

That would be fine by me.

fhahn added inline comments.
lib/Target/ARM/ARMTargetMachine.cpp
399

I've just put up a review D34958