Page MenuHomePhabricator

[LoopRotate] Don't rotate loops when the minsize attribute is present
Needs ReviewPublic

Authored by aykevl on Feb 9 2022, 7:26 AM.

Details

Summary

The main use for this patch is LTO. It is not (yet?) possible to set the size level (-Os, -Oz) in the linker, which means loops are still rotated even if -Oz is specified on the command line. Therefore, look at the function attribute instead of at the size level to determine whether to rotate loops for a given function.

For related discussion, see D72404.

Diff Detail

Event Timeline

aykevl created this revision.Feb 9 2022, 7:26 AM
aykevl requested review of this revision.Feb 9 2022, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2022, 7:26 AM
fhahn added a comment.Feb 9 2022, 9:03 AM

Should we do this when the optsize function attribute is set, not just when the minsize function attribute is set?

I don't think so. Disabling rotation is a heavy hammer, as it will disable a lot of other loop optimizations implicitly due to most transforms requiring rotated form.

llvm/lib/Transforms/Scalar/LoopRotation.cpp
54

I think we we add minsize handling here we should also drop the EnableHeaderDuplication flag set when instantiating the pass. This is more in line with what most other passes do.

Should we do this when the optsize function attribute is set, not just when the minsize function attribute is set?

I don't think so. Disabling rotation is a heavy hammer, as it will disable a lot of other loop optimizations implicitly due to most transforms requiring rotated form.

Ok, sounds reasonable. After checking again this is indeed what happens in the default pass pipeline (old PM and new PM).
(I think it might be reasonable to reduce the rotation threshold in -Os but that's a separate issue).

llvm/lib/Transforms/Scalar/LoopRotation.cpp
54

Sounds like a good simplification. However, there is a case where this would result in a difference in behavior: buildO1FunctionSimplificationPipeline unconditionally disables header duplication.

LPM1.addPass(LoopRotatePass(/* Disable header duplication */ true,
                            isLTOPreLink(Phase)));

So, should EnableHeaderDuplication really be removed, if this means -O1 will become a bit more like -O2?

Is this the correct way to go, or do we still need a patch like D72404 or D81223?

IMO this is preferable. As much as possible the opt passes should be honoring function attributes. It allows them to work seamlessly with LTO, and it also allows correct handling of LTO linking of functions built with different flags. There's still some legacy handling that uses flags passed down from the driver, but we should be migrating to attributes as much as possible.

llvm/lib/Transforms/Scalar/LoopRotation.cpp
54

I guess the question this raises is why we don't have a function attribute for -O1 as we do for -O0/-Os/-Oz. Probably this field should be left until such attribute is added, or the change is evaluated at -O1.

aykevl updated this revision to Diff 407781.EditedFeb 10 2022, 11:25 PM
aykevl edited the summary of this revision. (Show Details)

Ok, I've updated this patch to remove EnableHeaderDuplication.

Note that instead of duplicating headers at -O1, we might also remove LoopRotate at -O1 altogether? This would make sense to me, as I think vectorization shouldn't happen at -O1 anyway and -O1 generally only enables relatively simple transformations.

tejohnson added inline comments.Mar 2 2022, 10:17 AM
llvm/lib/Transforms/Scalar/LoopRotation.cpp
54

I don't think the removal of EnableHeaderDuplication should happen in this patch, which is about minsize. I would suggest splitting that change into a separate follow on patch.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 10:17 AM
aykevl added inline comments.Mar 12 2022, 9:22 AM
llvm/lib/Transforms/Scalar/LoopRotation.cpp
54

@tejohnson so you're saying I should revert this patch back to https://reviews.llvm.org/D119342?vs=on&id=407156? I removed EnableHeaderDuplication at the request of @fhahn.

fhahn added inline comments.Mar 14 2022, 5:29 AM
llvm/lib/Transforms/Scalar/LoopRotation.cpp
54

Sounds like a good simplification. However, there is a case where this would result in a difference in behavior: buildO1FunctionSimplificationPipeline unconditionally disables header duplication.

Oh right, that's unfortunate! Given that, it's probably better to keep EnableHeaderDuplication for now, as removing it is more complicated.

ormris removed a subscriber: ormris.May 16 2022, 11:21 AM
ormris added a subscriber: ormris.
aykevl updated this revision to Diff 439584.Thu, Jun 23, 6:05 PM
aykevl edited the summary of this revision. (Show Details)
  • revert EnableHeaderDuplication removal