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

[David Blaikie via llvm-commits]

On Fri, Jun 9, 2017 at 9:15 AM Florian Hahn via Phabricator <
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.


>
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170609/dde01743/attachment.html>