arsenm added inline comments.

================
Comment at: clang/lib/CodeGen/CGCall.cpp:2030-2031
+      bool EnabledForTarget = TEntry->second;
+      if (EnabledForTarget != EnabledForFunc)
+        return;
+    }
----------------
jmmartinez wrote:
> 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;
>     }
You find an incompatible feature and then discontinue processing any further 
features by early exiting. I expect this to act like an append to any features 
already present. The incompatibility is at an individual feature level, not the 
group 


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2034
+  }
+
+  FuncAttr.addAttribute("target-features", llvm::join(MergedFeatures, ","));
----------------
jmmartinez wrote:
> arsenm wrote:
> > Really it would be less bad if the incompatible functions were not imported 
> > rather than the backend pass
> I thought it was possible to have functions with incompatible features in the 
> same module. 
> e.g. one function compiled with some instruction set support, one without, 
> and an ifunc that resolves to one or the other.
> 
> Maybe it's not the case in the context of `-mlink-builtin-bitcode`?
The truth is this system isn't really well considered. We don't have real ifunc 
support and we probably shouldn't be using subtargets for cases with 
incompatible encodings 


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