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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:594
+    const Triple TT(TheModule.getTargetTriple());
+    if (TT.getArch() != Triple::thumb && TT.getArch() != Triple::thumbeb &&
+        TT.getArch() != Triple::arm && TT.getArch() != Triple::armeb)
----------------
javed.absar wrote:
> maybe you should add isARMorThumb to ADT/Triple.h and use it, much like isPS4(), isAndroid etc?
Maybe it would make sense to add an isThumb() and isARm() method to Triple. In particular, isThumb could be used at a couple of places. But ideally those checks should be replaced to use the ARMSubtargetFeatures.


================
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)
----------------
dblaikie wrote:
> 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)
There is no need to check if the feature is present, getFnAttribute returns an empty attribute in case the attribute does not exist.


================
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:
> 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")?
No, AFAIK target features are only checked during CodeGen, using a TargetMachine or Subtarget instance. I think initializing an ARMSubtarget instance in this case would be overkill. I've strengthened the check though, so it should not match if thumb-mode appears in a substring.

Re your second comment: yes the idea is that code generated by clang now sets +/-thumb-mode for all functions. For functions without a thumb-mode target feature and  a thumb triple, Thumb code should be generated, thus +thumb-mode is added. 


================
Comment at: test/Bitcode/thumb-mode-upgrade-arm.ll:1
+; RUN: llvm-dis -o - %s.bc | FileCheck %s
+
----------------
javed.absar wrote:
> Could the same check be achieved  using something like 'llvm-as < %s | llvm-dis ..."?
I think the reason to include a bitcode file is to make sure older versions of the bitcode format are handled correctly going forward


https://reviews.llvm.org/D34028