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

[silviu.baranga@arm.com via llvm-commits]

sbaranga 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:
> > 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.


http://reviews.llvm.org/D21432