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

[Nathan Huckleberry via Phabricator via llvm-commits]

Nathan-Huckleberry added inline comments.


================
Comment at: llvm/lib/Support/X86TargetParser.cpp:570
+    Prev = Bits;
+    for (unsigned i = CPU_FEATURE_MAX; i;)
+      if (Bits[--i])
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85257