[PATCH] D21432: [ARM] Do not test for CPUs, use SubtargetFeatures (Part 1). NFC

Diana Picus via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 22 04:04:57 PDT 2016


rovka added inline comments.

================
Comment at: lib/Target/ARM/ARM.td:137
@@ +136,3 @@
+// VFP to NEON.
+def FeatureNEONForFPMovs : SubtargetFeature<"neon-fpmovs", "UseNEONForFPMovs",
+                              "true", "Convert VMOVSR, VMOVRS, VMOVS to NEON">;
----------------
rengolin wrote:
> sbaranga wrote:
> > rengolin wrote:
> > > sbaranga wrote:
> > > > They are both core-specific optimizations, so it makes sense that they are enabled selectively:
> > > > 
> > > > FeatureNEONForFPMoves enables changing the execution domain for mov-like instructions from VFP to NEON and is a core-specific optimization, so it makes sense to me that it's only enabled on Cortex-A9 (unless we have additional data points).
> > > > 
> > > > FeatureNEONforFP forces almost all floating point operations to be performed with NEON instructions (which is a problem if we care about how denormals are handled), so it makes sense to me that it's only enabled for swift/cyclone.
> > > > 
> > > > I think the distinction is that FeatureNEONForFP uses NEON for VFP movs only when it minimizes the domain crossing between NEON and VFP, while FeatureNEONforFP  does this everywhere (for movs and everything else). I'm not sure how you could improve the naming though.
> > > So, just because one is a sub-set of the other, makes sense to have the same name + radical. But the extra behaviour (NEON/VFP barrier profitability) could be documented, at least in its comment.
> > > 
> > > About Swift's behaviour to ignore NEON subnormal problem, we have the same issue at the IR level (loop vectorizer), and I have "solved" the old way "ST->isTargetDarwin()", which is true at the platform level.
> > > 
> > > If you're still not happy with the naming, maybe, such a property could be called "FeatureIgnoreDenormalFP"?
> > Above it should have been "I think the distinction is that FeatureNEONForFPMoves uses NEON for VFP movs only when it minimizes the domain crossing between NEON and VFP, while FeatureNEONforFP does this everywhere (for movs and everything else). I'm not sure how you could improve the naming though.". Sorry if that was a little bit confusing.
> > 
> > I'm not too picky about the naming for either features, they could stay as they are (maybe with some added documentation). Just wanted to point out that the they way these heuristics are used is not an oversight and to give some context in case anyone could come up with a better name for these.
> > 
> > I'm happy with FeatureNEONforFP at the moment, I don't think there's any need to change it.
> > 
> > FWIW I think FeatureNEONforFP is not a superset of FeatureNEONForFPMoves. From a behavioural point of view, there's the heuristic difference. The implementations are also very different (the first being at ISel level, the second is done as part of post-RA optimization). But that's just my interpretation.
> > I'm happy with FeatureNEONforFP at the moment, I don't think there's any need to change it.
> 
> Ok, me too.
> 
> 
> > FWIW I think FeatureNEONforFP is not a superset of FeatureNEONForFPMoves. From a behavioural point of view, there's the heuristic difference. The implementations are also very different (the first being at ISel level, the second is done as part of post-RA optimization).
> 
> Well, it's about the meaning, not the behaviour. I think it's ok to have behaviour described on comments and meaning encoded in names.
Ok, thanks. I'll try to add more comments.

================
Comment at: lib/Target/ARM/ARMInstrInfo.td:329
@@ +328,3 @@
+
+def UseVMOVSR : Predicate<"Subtarget->preferVMOVSR() ||"
+                          "!Subtarget->useNEONForSinglePrecisionFP()">;
----------------
sbaranga wrote:
> Is there any reason not to test for useNEONForFPMovs instead?
Not really, just that this seems to be used at ISel level, whereas useNEONForFPMovs is used for execution domain optimization, so I wasn't sure they were in the same boat. I can change it to useNEONForFPMovs if you think that makes sense.

================
Comment at: lib/Target/ARM/ARMSubtarget.h:221-237
@@ -220,1 +220,19 @@
 
+  /// If true, if conversion may decide to leave some instructions unpredicated.
+  bool IsProfitableToUnpredicate;
+
+  /// If true, VMOV will be favored over VGETLNi32.
+  bool HasSlowVGETLNi32;
+
+  /// If true, VMOV will be favored over VDUP.
+  bool HasSlowVDUP32;
+
+  /// If true, VMOVSR will be favored over VMOVDRR.
+  bool PreferVMOVSR;
+
+  /// If true, ISHST barriers will be used for Release semantics.
+  bool PreferISHST;
+
+  /// If true, VMOVRS, VMOVSR and VMOVS will be converted from VFP to NEON.
+  bool UseNEONForFPMovs;
+
----------------
MatzeB wrote:
> I find member initializers a lot less error prone than having the initializers in a separate method. But we can fix that in another commit.
Ok, I'll try to look into that after I'm done with the subtarget features.


http://reviews.llvm.org/D21432





More information about the llvm-commits mailing list