[PATCH] [AArch64] Refactor AArch64NamedImmMapper to become dependent on subtarget features.

James Molloy james.molloy at arm.com
Thu Apr 16 03:21:05 PDT 2015


Hi Vlad,

Looks like a mostly mechanical change. I don't like one aspect of it, but if you change that you can commit.

Cheers,

James


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/AArch64/Utils/AArch64BaseInfo.h:285
@@ +284,3 @@
+    // empty AvailableForFeatures means "always-on"
+    template <typename T> bool isEqual(T Other) const;
+    template <typename T> bool isEqual(T Other, uint64_t FeatureBits) const {
----------------
I think this is overly convoluted for such a simple function. The same function name does different things depending on the overload (one checks Name and the other Value). I don't think this is intuitive.

Just simply:
    
    bool isNameEqual(std::string Other, uint64_t FeatureBits=~0ULL) {
      if (AvailableForFeatures && !(AvailableForFeatures & FeatureBits))
        return false;
      return Name == Other;
    }

And similarly for Value, would be more clear.

http://reviews.llvm.org/D8496

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list