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

[Florian Hahn via llvm-commits]

On 09/06/2017 18:18, David Blaikie wrote:
> 
> 
> On Fri, Jun 9, 2017 at 9:15 AM Florian Hahn via Phabricator 
> <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> 
>     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.
> 
> 
> 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.
> 
> 

There already exists code to deal with target-features [1], but it 
requires target-specific feature maps and I don't think we can use it in 
lib/Bitcode. I could not find any checks in code that would benefit from 
a general way to query for a target-feature in target-agnostic code.

[1] 
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/MC/SubtargetFeature.h#L93

>     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.
> 
> 
> 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?
> 
> 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.
> 
> Maybe that's fine... I dunno.
> 
> 
> 
>     ================
>     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
> 
> 
>