This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add OptMinSize Subtarget feature
ClosedPublic

Authored by samparker on Feb 6 2019, 2:49 AM.

Details

Summary

In many places in the backend, we like to know whether we optimising for code size and this is currently performed by checking the current function attributes. A subtarget is created on a per-function basis so it's possible to know if we're compiling for code size on construction. So, I've added the OptMinSize feature, set in the same way as soft float, and allows some of the APIs to be cleaned up. I think this should also enable us to make some informed decisions in the initialisation of ARMTargetLowering too.

Diff Detail

Repository
rL LLVM

Event Timeline

samparker created this revision.Feb 6 2019, 2:49 AM
fhahn added a reviewer: fhahn.Feb 6 2019, 3:09 AM
fhahn added a subscriber: fhahn.

Hi Sam! There are already 2 function attributes indicating whether to optimize for size (minsize, optsize). What is the benefit of adding another target-specific one? I agree that exposing optForMinSize() in STI makes sense, but cannot we just use the existing minsize attribute?

Hi Florian,

The main motivations here are:

  • remove the dependency of a MachineFunction on ARMSubtarget->useMovt so that we can query this function from ARMTTIImpl and other passes not using a MachineFunction.
  • use the subtarget to enable us to perform different setOperationActions.
  • use the subtarget to enable us to set different scheduling preferences.

cheers,

fhahn added a comment.Feb 6 2019, 3:38 AM

Hi Florian,

The main motivations here are:

  • remove the dependency of a MachineFunction on ARMSubtarget->useMovt so that we can query this function from ARMTTIImpl and other passes not using a MachineFunction.

Right, that makes sense. But wouldn't it work to set optForMinSize in getSubtargetImpl based on the attribute on the machine function, without adding a sub target attribute (it looks like there are at least a few member variables in ARMSubtarget, that not directly correspond to a sub target feature).

IIUC the new target feature has exactly the same semantics as the existing minsize. Having them both might lead to situations where the target feature is set but not the function attribute and some passes respect just one of them.

  • use the subtarget to enable us to perform different setOperationActions.
  • use the subtarget to enable us to set different scheduling preferences.

cheers,

Ah yes, good point! I'll make the change.

samparker updated this revision to Diff 185524.Feb 6 2019, 3:53 AM

Added method setOptMinSize which is set in getSubtargetImpl.

fhahn added inline comments.Feb 6 2019, 4:03 AM
lib/Target/ARM/ARMISelLowering.h
570 ↗(On Diff #185524)

As the implementation is a one-liner, I guess we could keep it inline here.

lib/Target/ARM/ARMSubtarget.h
197 ↗(On Diff #185524)

Maybe worth calling out that this is just a convenience field and is set based on the minsize function attribute?

715 ↗(On Diff #185524)

Do we anticipate STI users to change this setting? I think it would be better to set it directly in the constructor, if possible, to make sure STI.OptMinSize cannot diverge from the function attribute.

lib/Target/ARM/Thumb2SizeReduction.cpp
1131 ↗(On Diff #185524)

Now that we can just query it from STI, is it worth to have MinimizeSize as separate member in this class?

samparker marked 2 inline comments as done.Feb 6 2019, 6:19 AM
samparker added inline comments.
lib/Target/ARM/ARMISelLowering.h
570 ↗(On Diff #185524)

This causes a bunch of confusing declaration issues...

lib/Target/ARM/ARMSubtarget.h
715 ↗(On Diff #185524)

This doesn't work, I think because targets are cached in a map in TargetMachine so a new one isn't necessary constructed for each function.

samparker updated this revision to Diff 185538.Feb 6 2019, 6:19 AM

updated member description

samparker marked an inline comment as done.Feb 6 2019, 9:04 AM
samparker added inline comments.
lib/Target/ARM/ARMISelLowering.h
570 ↗(On Diff #185524)

There's some nasty dependencies between ARMTargetLowering and ARMSubtarget, so I can't do this.

efriedma added inline comments.Feb 6 2019, 9:44 AM
lib/Target/ARM/ARMTargetMachine.cpp
279 ↗(On Diff #185538)

You can't mutate the subtarget here; subtargets are only allowed to vary based on information in the key of SubtargetMap (or information that's invariant across the module). In particular, this will break with module passes, like the machine outliner.

The SubtargetMap is under the control of the target; you can change the key type if you want.

samparker updated this revision to Diff 185720.Feb 7 2019, 1:25 AM

Removed the setter and instead changed the subtarget map key.

fhahn accepted this revision.Feb 7 2019, 3:41 AM

LGTM, thanks! Please wait till tomorrow with committing, in case Eli has any additional comments.

lib/Target/ARM/ARMTargetMachine.cpp
267 ↗(On Diff #185720)

Might be worth calling out what is going on here in a comment.

This revision is now accepted and ready to land.Feb 7 2019, 3:41 AM
efriedma accepted this revision.Feb 7 2019, 12:19 PM
efriedma added a subscriber: rovka.

LGTM

lib/Target/ARM/ARMInstrInfo.td
726 ↗(On Diff #185720)

It should be possible to address this comment from https://reviews.llvm.org/rL351056, since this doesn't require a MachineFunction anymore, but I guess that can be a followup.

Thanks to you both.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 11:58 PM
rovka added inline comments.Feb 8 2019, 2:34 AM
lib/Target/ARM/ARMInstrInfo.td
726 ↗(On Diff #185720)

My thoughts exactly, I'll look into it next week if nobody else gets to it first :) Thanks!