[PATCH] D57812: [ARM] Add OptMinSize Subtarget feature

[Florian Hahn via Phabricator via llvm-commits]

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