arsenm added inline comments.

================
Comment at: lib/Basic/Targets.cpp:2059-2063
@@ +2058,7 @@
+
+  if (Has16BitInsts)
+    Features["16-bit-insts"] = true;
+
+  if (hasSMemRealTime)
+    Features["s-memrealtime"] = true;
+
----------------
echristo wrote:
> This is typically more of the "move the cpu checks down here" area from what 
> you'd have above. Also you're not calling the target independent version of 
> initFeatureMap - is that done on purpose?
Not sure what you mean exactly by "move the cpu checks down here". Do you mean 
setting all of the feature has* member variables should be moved out of the 
AMDGPUTargetInfo constructor + setCPU into here?

That was not done on purpose. I'm pretty confused about what all of these 
functions are for. I kind of expected all of this to automatically work from 
the target's set of defined features known from the backend. There seem to be a 
set of functions for manually parsing user specified features on a function and 
for those implied by the subtarget. Why is initFeatureMap separate from 
handleTargetFeatures? initFeatureMap seems to be almost the same thing with the 
addition of the CPU features. It's not clear to me what the difference between 
hasFeature and validateCpuSupports is supposed to mean, or why 
setFeatureEnabled is virtual.



http://reviews.llvm.org/D17516



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to