Quoting Ville Syrjälä (2018-02-09 15:12:58) > On Fri, Feb 09, 2018 at 04:48:43PM +0200, Ville Syrjälä wrote: > > On Fri, Feb 09, 2018 at 01:18:49PM +0200, Jani Nikula wrote: > > > On Thu, 08 Feb 2018, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > > > On Thu, Feb 08, 2018 at 05:13:02PM +0200, Mika Kuoppala wrote: > > > >> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: > > > >> > > > >> > Quoting Tvrtko Ursulin (2018-02-08 14:34:38) > > > >> >> > > > >> >> On 08/02/2018 14:22, Ville Syrjälä wrote: > > > >> >> > On Thu, Feb 08, 2018 at 01:06:05PM +0000, Tvrtko Ursulin wrote: > > > >> >> >> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > > >> >> >> > > > >> >> >> Coccinelle transformation: > > > >> >> >> > > > >> >> >> @@ > > > >> >> >> expression p, g; > > > >> >> >> @@ > > > >> >> >> ( > > > >> >> >> -INTEL_GEN(p) > g > > > >> >> >> +IS_GEN_GT(p, g) > > > >> >> > > > > >> >> > I think this stuff makes the code pretty close to illegible. > > > >> >> > In this particular case even more so because "GT" actually > > > >> >> > means something very different to us. > > > >> >> > > > >> >> Oh how true! And I did not realize it at all while writing it! :) > > > >> >> > > > >> >> Anyway, something like this, regardless of a name, is needed if people > > > >> >> want this to be effective. Since the checks have to be moved to known at > > > >> >> compile time. Or a completely different approach will be needed. > > > >> > > > > >> > IS_GEN_RANGE() doesn't cut it? > > > >> > > > > >> > > > >> IS_GEN_RANGE(8,9); > > > >> > > > >> short and readable > > > > > > > > 'if (IS_GEN_RANGE(...))' reads funny. IS_GEN_IN_RANGE() would be more > > > > englishy perhaps, but it looks a bit off to me for whatever reason. > > > > > > We already have IS_GEN(dev_priv, start, end) for inclusive ranges with > > > GEN_FOREVER as unbound start/end. There's no need to bikeshed the naming > > > further. (Except perhaps the GEN_FOREVER part.) > > > > > > With some macro vararg hacking, we could probably make that work for > > > IS_GEN(dev_priv, exact) too, to replace say IS_GEN5() with one macro if > > > we like. Just to make more code use similar constructs. > > > > > > > And it still doesn't tell you anything about inclusive vs. exlusive. > > > > So it just forces you to waste brain cells on mundane details when > > > > reading the code. IMO that's a fairly bad tradeoff. > > > > > > Agreed. I think this patch is by far the worst part in the > > > series. Especially the _GT, _GTE, _LT, _LTE macros are IMO unacceptable. > > > > > > Quick greping shows that we have much more inclusive than exclusive > > > range checks. > > > > I think the usual pattern has been inclusive start, exclusive end. That > > can help you think in terms of "this is when the feature appeared, and > > this is when it disappeared". But if it's hidden in a macro then I > > think exclusive might end up being rather confusing. > > > > > We could help our brains a bit by switching, uh, > > > exclusively to inclusive ranges. That's not perfect, by far, as some > > > checks naturally work better with < and >, but I think I'd rather have > > > that with IS_GEN() than a bunch of macros you have to stop to think > > > about. > > > > If only the compiler would be smart and be able to see that something > > like > > > > #define INTEL_GEN(dev_priv) (ilog2((dev_priv)->gen_mask & CONFIG_GEN_MASK)) > > > > if (INTEL_GEN(dev_priv) < 8) > > ... > > > > can never be true... > > > > Not sure it can't actually. But I assume people have actually tried > > that. > > Hmm... > > #define IS_GEN(dev_priv, expr) ((ilog2(GEN_MASK) expr) && \ > (ilog2((dev_priv)->gen_mask expr))) > > if (IS_GEN(dev_priv, >= 8)) > ... > > Would something like that actually work? There are a few ways in which we could map gen to 0 if the platform wasn't enabled at build time, but I haven't a better approach than Tvrtko's replacement of INTEL_GEN() with IS_GENx() or IS_GEN(x, y). The biggest problem with just mapping gen to 0 comes in then we need to add the 0 case to many if-else chains and switches (it makes writing IS_GEN(i915, < 8) hard as well) but worse it's hard to do it in a way that doesn't result in the compiler emitting instructions rather than resolving it at compile time. Still doesn't look pretty, and we need some more surgery (some static tables pulling in function references need pruning) and LTO. Doing build tests is easy enough, I hope we can get CI to do per-platform builds (as I think those are more likely to show errors). -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
