[PATCH] D17516: AMDGPU: Verify subtarget specific builtins

[Eric Christopher via cfe-commits]

echristo added a comment.

Replied inline, I hope this is helpful :)


================
Comment at: lib/Basic/Targets.cpp:2059-2063
@@ +2058,7 @@
+
+  if (Has16BitInsts)
+    Features["16-bit-insts"] = true;
+
+  if (hasSMemRealTime)
+    Features["s-memrealtime"] = true;
+
----------------
arsenm wrote:
> 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.
> 
Attempting to answer your questions, I'm mostly going phrase by phrase :)

a) Yes, I mean this.
b) None of this code is connected to the backend at all. There's some disagreement as to what level the front end should connect to the backend in this way. It's probably better left for another day.
c) Yes, these functions are all part of handling features that can be set on a function, or are the defaults coming in via the driver.
d) They're separate, sadly, because I couldn't get them together in the time I spent rewriting it. Take a look at the comment at Targets.cpp:8086 for a bit more detail.
e) validateCpuSupports is only used in one case right now and that's for a particular builtin that's only supported on one target. Go ahead and ignore that for your purposes now, if you later want to support it we can talk about it. 
f) I'm not sure why you care whether or not setFeatureEnabled is virtual, but we can discuss it if you'd like - and why you think it shouldn't be :)

Most of these functions have few (or one) use and it is in Targets.cpp. (Don't worry about the ones outside of Targets.cpp for now, that's part of the function attribute code which was the cause of the cleanup (I promise it was) and refactoring I did, but doesn't affect clang initialization.


http://reviews.llvm.org/D17516