[PATCH] D57812: [ARM] Add OptMinSize Subtarget feature
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 6 04:03:46 PST 2019
fhahn added inline comments.
================
Comment at: lib/Target/ARM/ARMISelLowering.h:570
- bool shouldExpandShift(SelectionDAG &DAG, SDNode *N) const override {
- if (DAG.getMachineFunction().getFunction().optForMinSize())
- return false;
- return true;
- }
+ bool shouldExpandShift(SelectionDAG &DAG, SDNode *N) const override;
----------------
As the implementation is a one-liner, I guess we could keep it inline here.
================
Comment at: lib/Target/ARM/ARMSubtarget.h:197
+ /// OptMinSize - True if we're optimising for minimum code size.
+ bool OptMinSize = false;
----------------
Maybe worth calling out that this is just a convenience field and is set based on the minsize function attribute?
================
Comment at: lib/Target/ARM/ARMSubtarget.h:715
bool isThumb() const { return InThumbMode; }
+ void setOptForMinSize(bool MinSize) { OptMinSize = MinSize; }
+ bool optForMinSize() const { return OptMinSize; }
----------------
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.
================
Comment at: lib/Target/ARM/Thumb2SizeReduction.cpp:1131
OptimizeSize = MF.getFunction().optForSize();
- MinimizeSize = MF.getFunction().optForMinSize();
+ MinimizeSize = STI->optForMinSize();
----------------
Now that we can just query it from STI, is it worth to have MinimizeSize as separate member in this class?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57812/new/
https://reviews.llvm.org/D57812
More information about the llvm-commits
mailing list