[PATCH] D85257: [X86] Optimize getImpliedDisabledFeatures & getImpliedEnabledFeatures

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Support/X86TargetParser.cpp:570
+    Prev = Bits;
+    for (unsigned i = CPU_FEATURE_MAX; i;)
+      if (Bits[--i])
----------------
Nathan-Huckleberry wrote:
> craig.topper wrote:
> > MaskRay wrote:
> > > MaskRay wrote:
> > > > craig.topper wrote:
> > > > > Is there something special about going in reverse here?
> > > > Backward iteration is more efficient.
> > > > 
> > > > If most "implies" edges are forward (i.e. from a smaller index to a larger index; if we organize features from older to newer), one iteration is going to finalize `Bits`. The second iteration will break the loop.
> > > Backward iteration is more efficient.
> > > 
> > > If most "implies" edges are backward (i.e. from a larger index to a smaller index; if we organize features from older to newer), one sweep is going to finalize Bits. The second sweep will make `Prev == Bits`.
> > The feature list is currently sorted with the first portion in an order defined by libgcc/compiler-rt. And the rest in alphabetical order. I believe libgcc recently made a change to share some code with their driver and now all bits are features are defined by libgcc. With a bunch in alphabetical order. I'm not sure if that means all features are officially supported by __builtin_cpu_supports in gcc now or not.
> > 
> > But I think the majority of the features that have dependencies are in the libgcc compatibility section. Certainly the longest portion of any chain of dependencies is there.
> I might be missing something with how the data is sorted, but breaking the loop early feels like we might miss cases. 
> ```
> C -> A,B
> D -> A
> E -> A,C
> F -> D
> ```
> If we had the above dependencies and enabled {E,F} wouldn't we not enable B?
What loop are we breaking early?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85257