[AArch64] Macro fusion of simple ALU ops with branches for Broadcom's Vulcan

[pgode]

Summary

Support for the macro fusion of simple ALU ops with branches for the Vulcan sub-target.

Patch by Meador Inge

[mcrosier]

testcases?

[rengolin]

lib/Target/AArch64/AArch64InstrInfo.cpp

1806 ↗

(On Diff #62853)

This looks like a job for table-gen?

[rengolin]

lib/Target/AArch64/AArch64InstrInfo.cpp

1806 ↗

(On Diff #62853)

Also, please, don't add more getProcFamily calls, use sub-target features.

[echristo]

Redo the patch please and add llvm-commits as a subscriber before you
publish the patch.

[pgode]

lib/Target/AArch64/AArch64InstrInfo.cpp

1806 ↗

(On Diff #62853)

For Cyclone, only subset of 6 instruction fusion cases are applicable, but for Vulcan, additional instructions are applicable, which may not be applicable for Cyclone.
So, to generalize this under one 'Subtarget Feature', getProcFamily call seems unavoidable. Please suggest.

Or, Is a new subtarget feature, such as 'FeatureMacroOpFusionVulcanSubset' a better option?

Also, I am not sure about the table-gen option and how it can be done there.

[rengolin]

lib/Target/AArch64/AArch64InstrInfo.cpp

1806 ↗

(On Diff #62853)

This is not a Cyclone vs Vulcan, but some things are more profitable than others, and it's possible, as you said, that this is the case for other cores, including Cyclone.

A new target feature is needed, but one with "vulcan" in its name, because that would defeat the purpose. We are getting rid of things like that, ex. "isLikeA9" because it's a trap.

The table-gen option would be to add a property to those instructions and use a check on that property here. I'll pass on the benefits of that apporach vs. this (other people can chime in), but this is orthogonal to the target feature discussion.

Bottom line is: we don't want *more* CPU checks.

[pgode]

lib/Target/AArch64/AArch64InstrInfo.cpp

1806 ↗

(On Diff #62853)

The approach of adding a new sub-feature for Macro-op fusion, by categorizing the instructions (my presumption) doesn't seem a good option. It will end up adding too many subfeature such as FeatureMacroOpFusionArith/FeatureMacroOpFusionLogical. Please correct me.

I approached the table-gen option of adding instruction property, similar to adding CheapAsAMov property. But, in MCID Flags, there are already 32 flags, 'new flag MacroOpFusable' becomes the 33rd flag. As there might be future flags as well, which someone might add, so we should use it as a 64-bit.

I am thinking of submitting a 'new diff' on this review by just enabling 'FeatureMacroOpFusion' (AArch64.td file modification) for Vulcan and let only ADDS, SUBS, ANDS get fused (default Subtarget feature behavior) and work on table-gen part for complete solution. Please suggest.

[pgode]

The approach of adding a new sub-feature for Macro-op fusion, by categorizing the instructions (my presumption) doesn't seem a good option. It will end up adding too many subfeature such as FeatureMacroOpFusionArith/FeatureMacroOpFusionLogical. Please correct me.

I approached the table-gen option of adding instruction property, similar to adding CheapAsAMov property. In MCID(MCInstrDesc) Flags, there are already 32 flags, 'new flag MacroOpFusable' becomes the 33rd flag. Though Flags is 'uint64_t', still I see a warning message 'left shift count >= width of type'.

I am thinking of submitting a 'new diff' on this review by just enabling 'FeatureMacroOpFusion' (AArch64.td file modification) for Vulcan and let only ADDS, SUBS, ANDS get fused (default Subtarget feature behavior) and work on table-gen part for complete solution. Please suggest.

[rengolin]

In D22042#477752, @pgode wrote:

I approached the table-gen option of adding instruction property, similar to adding CheapAsAMov property. In MCID(MCInstrDesc) Flags, there are already 32 flags, 'new flag MacroOpFusable' becomes the 33rd flag. Though Flags is 'uint64_t', still I see a warning message 'left shift count >= width of type'.

Hum, that's not good. We'll have to think about many of them, if we can turn them into properties, rather than features. There were some that could, maybe we need a larger re-factor than I was expecting.

I am thinking of submitting a 'new diff' on this review by just enabling 'FeatureMacroOpFusion' (AArch64.td file modification) for Vulcan and let only ADDS, SUBS, ANDS get fused (default Subtarget feature behavior) and work on table-gen part for complete solution. Please suggest.

I think you're right. This is the pragmatic approach and will give us time to work out a better way forward.

Thanks!
--renato

[pgode]

Updated diff.
Removed fusion of additional instructions for Vulcan.
Default instructions supported by 'Macrofusion subtarget feature' will be fused.

[rengolin]

Right, that looks good, thanks!

test/CodeGen/AArch64/misched-fusion.ll is already testing the flag itself, so we don't need additional tests.

cheers,
--renato

llvm/trunk/lib/Target/AArch64/AArch64.td

def ProcKryo    : SubtargetFeature<"kryo", "ARMProcFamily", "Kryo",
                                    FeaturePredictableSelectIsExpensive
                                    ]>;
 
 def ProcVulcan  : SubtargetFeature<"vulcan", "ARMProcFamily", "Vulcan",
                                    "Broadcom Vulcan processors", [
                                    FeatureCRC,
                                    FeatureCrypto,
                                    FeatureFPARMv8,
+                                   FeatureMacroOpFusion,
                                    FeatureNEON,
                                    FeaturePostRAScheduler,
                                    HasV8_1aOps]>;
 
 def : ProcessorModel<"generic", NoSchedModel, [
                      FeatureCRC,
                      FeatureFPARMv8,
                      FeatureNEON,