<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 9, 2017 at 9:15 AM Florian Hahn via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">fhahn added inline comments.<br>
<br>
<br>
================<br>
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:594<br>
+    const Triple TT(TheModule.getTargetTriple());<br>
+    if (TT.getArch() != Triple::thumb && TT.getArch() != Triple::thumbeb &&<br>
+        TT.getArch() != Triple::arm && TT.getArch() != Triple::armeb)<br>
----------------<br>
javed.absar wrote:<br>
> maybe you should add isARMorThumb to ADT/Triple.h and use it, much like isPS4(), isAndroid etc?<br>
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.<br>
<br>
<br>
================<br>
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:599-600<br>
+    std::string TargetFeatures;<br>
+    if (F.hasFnAttribute("target-features"))<br>
+      TargetFeatures = F.getFnAttribute("target-features").getValueAsString();<br>
+    if (TargetFeatures.find("thumb-mode") != std::string::npos)<br>
----------------<br>
dblaikie wrote:<br>
> 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)<br>
There is no need to check if the feature is present, getFnAttribute returns an empty attribute in case the attribute does not exist.<br>
<br>
<br>
================<br>
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:601<br>
+      TargetFeatures = F.getFnAttribute("target-features").getValueAsString();<br>
+    if (TargetFeatures.find("thumb-mode") != std::string::npos)<br>
+      return;<br>
----------------<br>
dblaikie wrote:<br>
> dblaikie wrote:<br>
> > 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<br>
> 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")?<br>
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.<br></blockquote><div><br>I'd wonder if the code in ARMSubtarget could be refactored into something generically reusable (for splitting and examining the attributes) that could be used here and there, perhaps.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div><br>Should this be a verifier check/invariant that modern bitcode (I guess there's no version number, so no way to identify the old versus the new) & textual IR have +/-thumb on all functions?<br><br>I'm not sure how situations like this are usually handled in the verifier/upgrade where the semantics haven't changed, but it won't roundtrip through bitcode as such - it'll get sort of upgraded to a newer representation of the same semantics.<br><br>Maybe that's fine... I dunno.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: test/Bitcode/thumb-mode-upgrade-arm.ll:1<br>
+; RUN: llvm-dis -o - %s.bc | FileCheck %s<br>
+<br>
----------------<br>
javed.absar wrote:<br>
> Could the same check be achieved  using something like 'llvm-as < %s | llvm-dis ..."?<br>
I think the reason to include a bitcode file is to make sure older versions of the bitcode format are handled correctly going forward<br>
<br>
<br>
<a href="https://reviews.llvm.org/D34028" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34028</a><br>
<br>
<br>
<br>
</blockquote></div></div>