This patch adds a macro fusion using CodeGen/MacroFusion.cpp to pair AES
instructions back to back and adds FeatureFuseAES to enable the feature.
Details
Diff Detail
Event Timeline
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. | |
lib/Target/ARM/ARMMacroFusion.cpp | ||
42 | Should this check be upfront so it comes out immediately? In fact, to TargetMachine.cpp, for efficiency ? DAG->addMutation(createARMMacroFusionDAGMutation()); | |
lib/Target/ARM/CMakeLists.txt | ||
52 | why add twice? |
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. |
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. |
lib/Target/ARM/ARMTargetMachine.cpp | ||
---|---|---|
399 | In light of that, should 'shouldScheduleAdjacent' be renamed 'shouldScheduleAdjacentAES' ? |
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.
lib/Target/ARM/ARMTargetMachine.cpp | ||
---|---|---|
399 | That would be fine by me. |
nitpick - Formatting seems different from rest of defs in this file.
... : SubtargetFeature<"fuse-aes", "HasFuseAES", "true",