[PATCH] D34028: [Bitcode] Add thumb-mode to target-features in metadata loader.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:599-600
+    std::string TargetFeatures;
+    if (F.hasFnAttribute("target-features"))
+      TargetFeatures = F.getFnAttribute("target-features").getValueAsString();
+    if (TargetFeatures.find("thumb-mode") != std::string::npos)
----------------
Probably cheaper to get the attribute & test if it succeeded, than to test then get separately (one lookup rather than two). I'd check other API uses to see if there's a good idiom for a single query+test sort of thing as it's not readily apparent to me from the API (I'd expect Attribute to have an explicit operator bool, but it doesn't look like it has one)


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:601
+      TargetFeatures = F.getFnAttribute("target-features").getValueAsString();
+    if (TargetFeatures.find("thumb-mode") != std::string::npos)
+      return;
----------------
Is this how other target features are tested for in other parts of the codebase? I'd be worried that 'thumb-mode' could appear as a substring of a target feature and cause this to have a false positive


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:601
+      TargetFeatures = F.getFnAttribute("target-features").getValueAsString();
+    if (TargetFeatures.find("thumb-mode") != std::string::npos)
+      return;
----------------
dblaikie wrote:
> Is this how other target features are tested for in other parts of the codebase? I'd be worried that 'thumb-mode' could appear as a substring of a target feature and cause this to have a false positive
So is the idea that all new bitcode for ARM targets will have explicit -thumb-mode or +thumb-mode, so you'll know when upgrading that you're not overriding a default (eg: if you see a thumb triple but none of the functions have thumb-mode that doesn't mean each function was attributed with "not thumb")?


https://reviews.llvm.org/D34028