<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
<div class="moz-cite-prefix">On 09/04/2017 03:57 AM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAAwGriEB6PfJuz40XcL9MzWfi8AHhWcamTN9PX7vPw7UoQqGnQ@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr">On Sun, Sep 3, 2017 at 10:41 PM Hal Finkel via
llvm-commits <<a moz-do-not-send="true"
href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<p>Nevertheless, I think that you've convinced me that
this is a least-bad solution. I'll want a flag
preserving the old behavior. Something like
-fwiden-bitfield-accesses (modulo bikeshedding).<br>
</p>
</div>
</blockquote>
<div>Several different approaches have been discussed in this
thread, I'm not sure what you mean about "least-bad
solution"...</div>
<div><br>
</div>
<div>I remain completely unconvinced that we should change the
default behavior. At most, I'm not strongly opposed to
adding an attribute that indicates "please try to use narrow
loads for this bitfield member" and is an error if applied
to a misaligned or non-byte-sized bitfield member.</div>
</div>
</div>
</blockquote>
<br>
I like this solution too (modulo the fact that I dislike all of
these solutions). Restricting this only to affecting the loads, and
not the stores, is also an interesting thought. The attribute could
also be on the access itself (which, at least from the theoretical
perspective, I'd prefer).<br>
<br>
<blockquote
cite="mid:CAAwGriEB6PfJuz40XcL9MzWfi8AHhWcamTN9PX7vPw7UoQqGnQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> But I remain strongly opposed to changing the default
behavior. We have one or two cases that regress and are
easily addressed by source changes (either to not use
bitfields or to use an attribute). I don't think that is
cause to change the lowering Clang has used for years and
potentially regress many other use cases.</div>
</div>
</div>
</blockquote>
<br>
I have mixed feelings about all of the potential fixes here. To walk
through my thoughts on this:<br>
<br>
1. I don't like any solutions that require changes affecting source
level semantics. Something that the compiler does automatically is
fine, as is an attribute.<br>
<br>
2. Next, regarding default behavior, we have a trade off:<br>
<br>
A. Breaking apart the accesses, as proposed here, falls into the
category of "generally, it makes everything a little bit slower."
But it's worse than that because it comes on a spectrum. I can
easily construct variants of the provided benchmark which make the
separate loads have a bad performance penalty. For example:<br>
<br>
<tt>$ cat ~/tmp/3m.cpp</tt><tt><br>
</tt><tt>class A {</tt><tt><br>
</tt><tt>public:</tt><tt><br>
#ifdef BF<br>
</tt><tt> unsigned long f7:8;</tt><tt><br>
</tt><tt> unsigned long f6:8;</tt><tt><br>
</tt><tt> unsigned long f5:8;</tt><tt><br>
</tt><tt> unsigned long f4:8;</tt><tt><br>
</tt><tt> unsigned long f3:8;</tt><tt><br>
</tt><tt> unsigned long f2:8;</tt><tt><br>
</tt><tt> unsigned long f1:8;</tt><tt><br>
</tt><tt> unsigned long f0:8;</tt><tt><br>
#else<br>
unsigned char f7;<br>
unsigned char f6;<br>
unsigned char f5;<br>
unsigned char f4;<br>
unsigned char f3;<br>
unsigned char f2;<br>
unsigned char f1;<br>
unsigned char f0;<br>
#endif<br>
</tt><tt>};</tt><tt><br>
</tt><tt>A a;</tt><tt><br>
</tt><tt>bool b;</tt><tt><br>
</tt><tt>unsigned long N = 1000000000;</tt><tt><br>
</tt><tt><br>
</tt><tt>__attribute__((noinline))</tt><tt><br>
</tt><tt>void foo() {</tt><tt><br>
</tt><tt> a.f4 = 3;</tt><tt><br>
</tt><tt>}</tt><tt><br>
</tt><tt><br>
</tt><tt>__attribute__((noinline))</tt><tt><br>
</tt><tt>void goo() {</tt><tt><br>
</tt><tt> b = (a.f0 == 0 && a.f1 == (unsigned char)-1
&&</tt><tt><br>
</tt><tt> a.f2 == 0 && a.f3 == 0 && a.f4 == 0
&& a.f5 == 0 &&</tt><tt><br>
</tt><tt> a.f6 == 0 && a.f7 == (unsigned char)-1);</tt><tt><br>
</tt><tt>}</tt><tt><br>
</tt><tt><br>
</tt><tt>int main() {</tt><tt><br>
</tt><tt> unsigned long i;</tt><tt><br>
</tt><tt> foo();</tt><tt><br>
</tt><tt> for (i = 0; i < N; i++) {</tt><tt><br>
</tt><tt> goo();</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt>}</tt><br>
<br>
Run this and you'll find that our current scheme, on Haswell,
beats the separate-loads scheme by nearly 60% (e.g., 2.77s for
separate loads vs. 1.75s for the current bitfield lowering).<br>
<br>
So, how common is it to have a bitfield with a large number of
fields that could be loaded separately (i.e. have the right size and
alignment) and have code that loads a large number of them like this
(i.e. without other work to hide the relative costs)? I have no
idea, but regardless, there is definitely a high-cost end to this
spectrum.<br>
<br>
B. However, our current scheme can trigger expensive architectural
hazards. Moreover, there's no local after-the-fact analysis that can
fix this consistently. I think that Wei has convincingly
demonstrated both of these things. How common is this? I have no
idea. More specifically, how do the relative costs of hitting these
hazards compare to the costs of the increased number of loads under
the proposed scheme? I have no idea (and this certainly has a
workload-dependent answer).<br>
<br>
C. This situation seems unlikely to change in the future: it seems
like a physics problem. The data surrounding the narrower store is
simply not in the pipeline to be matched with the wider load.
Keeping the data in the pipeline would have extra costs, perhaps
significant. I'm guessing the basic structure of this hazard is here
to stay.<br>
<br>
D. In the long run, could this be a PGO-driven decision? Yes, and
this seems optimal. It would depend on infrastructure enhancements,
and critically, the hardware having the right performance counter(s)
to sample.<br>
<br>
So, as to the question of what to do right now, I have two thoughts:
1) All of the solutions will be bad for someone. 2) Which is a
least-bad default depends on the workload. Your colleagues seem to
be asserting that, for Google, the separate loads are least bad
(and, FWIW, you're more likely to have hot code like this than I
am). This is definitely an issue on which reasonable people can
disagree. In the end, I'll begrudgingly agree that this should be an
empirical decision. We should have some flag/pragma/attribute/etc.
to allow selection of the other method.<br>
<br>
Regardless, we should continue to use the current lowering when a
sanitizer is enabled.<br>
<br>
Any, hey, the rest of cfe-dev, please feel fee to express some
opinion on the matter!<br>
<br>
-Hal<br>
<pre class="moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</body>
</html>