jmmartinez added inline comments.

================
Comment at: clang/lib/CodeGen/CGCall.cpp:2017
+    for (StringRef Feature : llvm::split(FFeatures.getValueAsString(), ',')) {
+      bool EnabledForFunc = Feature[0] == '+';
+      StringRef Name = Feature.substr(1);
----------------
arsenm wrote:
> Do you need to guard against empty string?
I checked and I saw some tests without it. I'm adding the condition for it.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2018
+      bool EnabledForFunc = Feature[0] == '+';
+      StringRef Name = Feature.substr(1);
+      auto TEntry = TFeatures.find(Name);
----------------
arsenm wrote:
> consume_front
I used drop_front(1) instead, consume_front would only work with "+" and not 
drop "-".


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2030-2031
+      bool EnabledForTarget = TEntry->second;
+      if (EnabledForTarget != EnabledForFunc)
+        return;
+    }
----------------
arsenm wrote:
> Early return breaks the other features
I did not understand this remark.

If the features are not compatible, we do not add a "target-features" entry in 
the new "FuncAttrs". Then, the old "target-features" entry is kept in the 
Function coming from the builtin.

If you think it would be better to set the target-features in FuncAttrs to the 
old value in any case. If that's the case I've added the following code:

    if (EnabledForTarget != EnabledForFunc) {
        FuncAttr.addAttribute(FFeatures);
        return;
    }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159206/new/

https://reviews.llvm.org/D159206

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D159... Matt Arsenault via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Matt Arsenault via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Matt Arsenault via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits
    • [PATCH]... Yaxun Liu via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez Caamaño via Phabricator via cfe-commits

Reply via email to