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",