This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Allow unrolling if the unrolled size does not exceed loop size.
ClosedPublic

Authored by fhahn on Apr 4 2019, 7:07 AM.

Details

Summary

In the following cases, unrolling can be beneficial, even when
optimizing for code size:

  1. very low trip counts
  2. potential to constant fold most instructions after fully unrolling.

We can unroll in those cases, by setting the unrolling threshold to the
loop size. This might highlight some cost modeling issues and fixing
them will have a positive impact in general.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Apr 4 2019, 7:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 7:07 AM
paquette accepted this revision.Apr 4 2019, 9:11 AM

LGTM. Just a couple little comments, but none of them are critical:

  • In D60266 you have some code size numbers; it would be nice to have something similar here.
  • It would be nice to have a test with just the optsize attribute, since optForSize() will also return true if only that attribute is set.
  • If the intention here is only -Oz/minsize, you might want to replace the call to optForSize() with optForMinSize(). This seems like an appropriate change for -Os/optsize though, so I don't think it's necessary.
This revision is now accepted and ready to land.Apr 4 2019, 9:11 AM
fhahn added a comment.Apr 4 2019, 9:22 AM

LGTM. Just a couple little comments, but none of them are critical:

  • In D60266 you have some code size numbers; it would be nice to have something similar here.

Right, I should have said something here. Without D60266, there was no change in code size on the set of benchmarks, most likely because loop-rotate is not run and they are not in the required form for unrolling to happen.

  • It would be nice to have a test with just the optsize attribute, since optForSize() will also return true if only that attribute is set.

I guess it would be sufficient to drop minsize from the attribute list, assuming minsize always come with optsize?

  • If the intention here is only -Oz/minsize, you might want to replace the call to optForSize() with optForMinSize(). This seems like an appropriate change for -Os/optsize though, so I don't think it's necessary.

Yep, I think it should be enabled for both -Os/-Oz.

Right, I should have said something here. Without D60266, there was no change in code size on the set of benchmarks, most likely because loop-rotate is not run and they are not in the required form for unrolling to happen.

Ok, cool!

I guess it would be sufficient to drop minsize from the attribute list, assuming minsize always come with optsize?

minsize only comes with optsize when your function is effectively -Oz. If your function is -Os, it will only have optsize.

So, I would keep them both since -Oz behaviour is more important here (IMO). If you want to test both I guess you'll have to duplicate the test, sadly.

This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Apr 17 2019, 8:56 AM

Right, I should have said something here. Without D60266, there was no change in code size on the set of benchmarks, most likely because loop-rotate is not run and they are not in the required form for unrolling to happen.

Ok, cool!

I guess it would be sufficient to drop minsize from the attribute list, assuming minsize always come with optsize?

minsize only comes with optsize when your function is effectively -Oz. If your function is -Os, it will only have optsize.

So, I would keep them both since -Oz behaviour is more important here (IMO). If you want to test both I guess you'll have to duplicate the test, sadly.

Thanks, I've added a test that uses { midsize optsize } and switched the rest to {optsize}