This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Implement target hook function to decide folding (mul (add x, c1), c2)
ClosedPublic

Authored by benshi001 on Sep 1 2021, 7:31 PM.

Details

Summary

Prevent the folding in DAGCombine if it leads to worse code.

Diff Detail

Event Timeline

benshi001 created this revision.Sep 1 2021, 7:31 PM
benshi001 requested review of this revision.Sep 1 2021, 7:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2021, 7:31 PM
benshi001 updated this revision to Diff 370142.Sep 1 2021, 7:32 PM

I have troubles with thumb and 'arm without movt/movw', so let me submit the partial change, which leads to better code for 'arm with movt/movw'.

And mark the thumb and 'arm without movt/movw' as TODOs.

benshi001 updated this revision to Diff 370230.Sep 2 2021, 4:44 AM
benshi001 updated this revision to Diff 370469.Sep 2 2021, 7:03 PM

Saving it for later sounds OK to me, but what made Thumb difficult out of interest?

Saving it for later sounds OK to me, but what made Thumb difficult out of interest?

Sorry I am only familiar with ARM ISA. :(

I need extra time to learn about Thumb.

Could this use ConstantMaterializationCost from ARMBaseInstrInfo for the costs of materializing the constants? That way it should just work with any architecture or constants, with some simple checking for which will produce the lowest total cost. It might need to include isLegalAddImmediate too, but that should be handling Thumb and Arm already too.

RKSimon added inline comments.Sep 3 2021, 8:47 AM
llvm/lib/Target/ARM/ARMISelLowering.h
522 ↗(On Diff #370469)

why are we passing SDValue by const-ref? These things are lightweight enough we normally pass SDValue by value.

benshi001 marked an inline comment as done.Sep 3 2021, 8:29 PM
benshi001 added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.h
522 ↗(On Diff #370469)

No special reason for that. Actually I did not pay attention to that const-ref in my previous patch https://reviews.llvm.org/D107711

benshi001 marked an inline comment as done.Sep 3 2021, 8:29 PM
benshi001 updated this revision to Diff 370700.Sep 3 2021, 10:34 PM

Could this use ConstantMaterializationCost from ARMBaseInstrInfo for the costs of materializing the constants? That way it should just work with any architecture or constants, with some simple checking for which will produce the lowest total cost. It might need to include isLegalAddImmediate too, but that should be handling Thumb and Arm already too.

Thanks for your advice. I have changed to using ConstantMaterializationCost, it does work for all arm/thumb targets.

One more concern, this change generates better code for most cases, except the ARMV5/ARMV6 tags in @test_urem_vec in llvm/test/CodeGen/ARM/urem-seteq-illegal-types.ll. The total number of ARMV5 instructions decreases from 38 to 36, and ARMV6 also decreases from 38 to 36. It looks like improvement than regression. But I am sure you can accept such a large change.

So in my previous patch revision, I left ARMv5/v6 as TODO and only did for subtargets with movw/movt.

What is the best form do you think? I can fall back to thumb + arm_with_movt, which I thought to be better.

The urem cases look OK to me. What do the other tests look like when they have been rebased on top of D109123?

benshi001 updated this revision to Diff 370982.Sep 6 2021, 7:11 PM

The urem cases look OK to me. What do the other tests look like when they have been rebased on top of D109123?

Done. I have rebased on top of D109123 .

The urem cases look OK to me. What do the other tests look like when they have been rebased on top of D109123?

Done. I have rebased on top of D109123 .

Sometimes the total instruction amount does not change, but an item in the constant pool at the end is eliminated.

dmgreen accepted this revision.Sep 7 2021, 12:22 AM

Yeah looks like a good improvement.

Thanks for the patch. LGTM

This revision is now accepted and ready to land.Sep 7 2021, 12:22 AM