On Mon, 21 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> On 21/11/2016 09:27, Jani Nikula wrote:
>> On Fri, 18 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
>>> On 18/11/2016 14:20, Jani Nikula wrote:
>>>> The platform flags in device info are (mostly) mutually
>>>> exclusive. Replace the flags with an enum. Add the platform enum also
>>>> for platforms that previously didn't have a flag, and give them codename
>>>> logging in dmesg.
>>>
>>> It just saddens me a bit that it prevents the compiler optimisation of
>>> our IS_THIS || IS_THAT || IS_TAT ugliness. :I On the basis on that I
>>> cannot quite make myself to support it, although it conceptually does
>>> make more sense.
>>
>> Personally I think making more sense conceptually trumps possible
>> compiler optimizations, especially when the code paths where this
>> actually matters are extremely rare. (Can you even point me at an
>> example where this makes a difference?)
>
> Without thinking deeply, all instances like IS_HASWELL || IS_BROADWELL,
> IS_SKYLAKE || IS_KABYLAKE, or IS_VALLEYVIEW || IS_CHERRYVIEW. Quick grep
> shows that there is at least around 170 instances of those. There might
> be more, I don't know. At least the ones I listed all currently
> translate to a single conditional in the generated code.
>
> I am not saying these are all on performance critical paths or anything,
> just that on balance, for me the sweetness of the optimisation cancels
> out the ugliness of the code (which is often unavoidable). Losing that
> and ending up with compare ladders in the code would just be a bit sad.
It doesn't persuade me much if something enables sweet optimizations
that nobody can perceive in real life.
Please also note that this brings decent codename debug prints to dmesg,
for all platforms, instead of a bunch of irrelevant is_platform = no for
every platform.
>> That said, does doing something silly like this make a difference:
>>
>> enum intel_platform {
>> INTEL_PLATFORM_UNINITIALIZED = 0,
>> INTEL_I830 = BIT(0),
>> INTEL_I845G = BIT(1),
>> INTEL_I85X = BIT(2),
>> INTEL_I865G = BIT(3),
>> INTEL_I915G = BIT(4),
>> INTEL_I915GM = BIT(5),
>> INTEL_I945G = BIT(6),
>> INTEL_I945GM = BIT(7),
>> INTEL_G33 = BIT(8),
>> INTEL_G4X = BIT(9),
>> INTEL_PINEVIEW = BIT(10),
>> INTEL_BROADWATER = BIT(11),
>> INTEL_CRESTLINE = BIT(12),
>> INTEL_IRONLAKE = BIT(13),
>> INTEL_SANDYBRIDGE = BIT(14),
>> INTEL_IVYBRIDGE = BIT(15),
>> INTEL_VALLEYVIEW = BIT(16),
>> INTEL_CHERRYVIEW = BIT(17),
>> INTEL_HASWELL = BIT(18),
>> INTEL_BROADWELL = BIT(19),
>> INTEL_SKYLAKE = BIT(20),
>> INTEL_BROXTON = BIT(21),
>> INTEL_KABYLAKE = BIT(22),
>> };
>
> I could live with that. Probably best to compare the code size before
> and after just to verify there are no surprises.
current nightly:
text data bss dec hex filename
1254149 43528 2048 1299725 13d50d drivers/gpu/drm/i915/i915.ko
original platform enum patch:
text data bss dec hex filename
1254456 43529 2048 1300033 13d641 drivers/gpu/drm/i915/i915.ko
the same with BIT() for platform enums:
text data bss dec hex filename
34811672 43529 2048 34857249 213e121 drivers/gpu/drm/i915/i915.ko
Holy cow, what happened there?!
Anyway the nightly -> original patch change is just a 300 byte increase
in text size.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
