[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

Hal Finkel via cfe-commits cfe-commits at lists.llvm.org
Sun Sep 3 22:46:15 PDT 2017


On 09/03/2017 11:22 PM, Wei Mi wrote:
> On Sun, Sep 3, 2017 at 8:55 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> On 09/03/2017 10:38 PM, Xinliang David Li wrote:
>>
>> Store forwarding stall cost is usually much higher compared with a load
>> hitting L1 cache. For instance, on Haswell,  the latter is ~4 cycles, while
>> the store forwarding stalls cost about 10 cycles more than a successful
>> store forwarding, which is roughly 15 cycles. In some scenarios, the store
>> forwarding stalls can be as high as 50 cycles. See Agner's documentation.
>>
>>
>> I understand. As I understand it, there are two potential ways to fix this
>> problem:
>>
>>   1. You can make the store wider (to match the size of the wide load, thus
>> permitting forwarding).
>>   2. You can make the load smaller (to match the size of the small store,
>> thus permitting forwarding).
>>
>> At least in this benchmark, which is a better solution?
>>
>> Thanks again,
>> Hal
>>
> For this benchmark, smaller load is better. On my sandybridge desktop,
> wider store is 3.77s, smaller load is 3.45s. If store forwarding is
> blocked, it costs 6.9s.
>
> However, we don't have good way to narrow the load matching the store
> shrinking because the field information has been lost.  For the IR
> below:
>
> define void @_Z3goov() local_unnamed_addr #0 {
>    %1 = load i64, i64* bitcast (%class.A* @a to i64*), align 8
>    %2 = and i64 %1, 0xffffff00000000
>    %3 = icmp eq i64 %2, 0xffff00000000
>    %4 = zext i1 %3 to i8
>    store i8 %4, i8* @b, align 1, !tbaa !2
>    ret void
> }
>
> We know the 24bits range from bit 32 to bit 56 of @a are accessed, but
> we don't know whether the 24bits ranges contain 8bits + 16bits
> bitfields, or 16bits + 8bits bitfields, or 8bit + 8bit + 8bit
> bitfields. Once the load shrinking done locally is inconsistent with
> store shrinking, we will have store forwarding issue and will suffer
> from huge regression.

Understood. This is a convincing argument. The cost of splitting the 
loads seems not high, at least not in isolation.

Thanks again,
Hal

>
> Thanks,
> Wei.
>
>
>
>>
>> In other words, the optimizer needs to be taught to avoid defeating  the HW
>> pipeline feature as much as possible.
>>
>> David
>>
>> On Sun, Sep 3, 2017 at 6:32 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>>
>>> On 09/03/2017 03:44 PM, Wei Mi wrote:
>>>> On Sat, Sep 2, 2017 at 6:04 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>>>> On 08/22/2017 10:56 PM, Wei Mi via llvm-commits wrote:
>>>>>> On Tue, Aug 22, 2017 at 7:03 PM, Xinliang David Li <davidxl at google.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via Phabricator
>>>>>>> <reviews at reviews.llvm.org> wrote:
>>>>>>>> chandlerc added a comment.
>>>>>>>>
>>>>>>>> I'm really not a fan of the degree of complexity and subtlety that
>>>>>>>> this
>>>>>>>> introduces into the frontend, all to allow particular backend
>>>>>>>> optimizations.
>>>>>>>>
>>>>>>>> I feel like this is Clang working around a fundamental deficiency in
>>>>>>>> LLVM
>>>>>>>> and we should instead find a way to fix this in LLVM itself.
>>>>>>>>
>>>>>>>> As has been pointed out before, user code can synthesize large
>>>>>>>> integers
>>>>>>>> that small bit sequences are extracted from, and Clang and LLVM
>>>>>>>> should
>>>>>>>> handle those just as well as actual bitfields.
>>>>>>>>
>>>>>>>> Can we see how far we can push the LLVM side before we add complexity
>>>>>>>> to
>>>>>>>> Clang here? I understand that there remain challenges to LLVM's
>>>>>>>> stuff,
>>>>>>>> but I
>>>>>>>> don't think those challenges make *all* of the LLVM improvements off
>>>>>>>> the
>>>>>>>> table, I don't think we've exhausted all ways of improving the LLVM
>>>>>>>> changes
>>>>>>>> being proposed, and I think we should still land all of those and
>>>>>>>> re-evaluate how important these issues are when all of that is in
>>>>>>>> place.
>>>>>>>
>>>>>>> The main challenge of doing  this in LLVM is that inter-procedural
>>>>>>> analysis
>>>>>>> (and possibly cross module) is needed (for store forwarding issues).
>>>>>>>
>>>>>>> Wei, perhaps you can provide concrete test case to illustrate the
>>>>>>> issue
>>>>>>> so
>>>>>>> that reviewers have a good understanding.
>>>>>>>
>>>>>>> David
>>>>>> Here is a runable testcase:
>>>>>> -------------------- 1.cc ------------------------
>>>>>> class A {
>>>>>> public:
>>>>>>      unsigned long f1:2;
>>>>>>      unsigned long f2:6;
>>>>>>      unsigned long f3:8;
>>>>>>      unsigned long f4:4;
>>>>>> };
>>>>>> A a;
>>>>>> unsigned long b;
>>>>>> unsigned long N = 1000000000;
>>>>>>
>>>>>> __attribute__((noinline))
>>>>>> void foo() {
>>>>>>      a.f3 = 3;
>>>>>> }
>>>>>>
>>>>>> __attribute__((noinline))
>>>>>> void goo() {
>>>>>>      b = a.f3;
>>>>>> }
>>>>>>
>>>>>> int main() {
>>>>>>      unsigned long i;
>>>>>>      for (i = 0; i < N; i++) {
>>>>>>        foo();
>>>>>>        goo();
>>>>>>      }
>>>>>> }
>>>>>> ------------------------------------------------------------
>>>>>> Now trunk takes about twice running time compared with trunk + this
>>>>>> patch. That is because trunk shrinks the store of a.f3 in foo (Done by
>>>>>> DagCombiner) but not shrink the load of a.f3 in goo, so store
>>>>>> forwarding will be blocked.
>>>>>
>>>>> I can confirm that this is true on Haswell and also on an POWER8.
>>>>> Interestingly, on a POWER7, the performance is the same either way (on
>>>>> the
>>>>> good side). I ran the test case as presented and where I replaced f3
>>>>> with a
>>>>> non-bitfield unsigned char member. Thinking that the POWER7 result might
>>>>> be
>>>>> because it's big-Endian, I flipped the order of the fields, and found
>>>>> that
>>>>> the version where f3 is not a bitfield is faster than otherwise, but
>>>>> only by
>>>>> 12.5%.
>>>>>
>>>>> Why, in this case, don't we shrink the load? It seems like we should
>>>>> (and it
>>>>> seems like a straightforward case).
>>>>>
>>>>> Thanks again,
>>>>> Hal
>>>>>
>>>> Hal, thanks for trying the test.
>>>>
>>>> Yes, it is straightforward to shrink the load in the test. I can
>>>> change the testcase a little to show why it is sometime difficult to
>>>> shrink the load:
>>>>
>>>> class A {
>>>> public:
>>>>     unsigned long f1:16;
>>>>     unsigned long f2:16;
>>>>     unsigned long f3:16;
>>>>     unsigned long f4:8;
>>>> };
>>>> A a;
>>>> bool b;
>>>> unsigned long N = 1000000000;
>>>>
>>>> __attribute__((noinline))
>>>> void foo() {
>>>>     a.f4 = 3;
>>>> }
>>>>
>>>> __attribute__((noinline))
>>>> void goo() {
>>>>     b = (a.f4 == 0 && a.f3 == (unsigned short)-1);
>>>> }
>>>>
>>>> int main() {
>>>>     unsigned long i;
>>>>     for (i = 0; i < N; i++) {
>>>>       foo();
>>>>       goo();
>>>>     }
>>>> }
>>>>
>>>> For the load a.f4 in goo, it is diffcult to motivate its shrink after
>>>> instcombine because the comparison with a.f3 and the comparison with
>>>> a.f4 are merged:
>>>>
>>>> define void @_Z3goov() local_unnamed_addr #0 {
>>>>     %1 = load i64, i64* bitcast (%class.A* @a to i64*), align 8
>>>>     %2 = and i64 %1, 0xffffff00000000
>>>>     %3 = icmp eq i64 %2, 0xffff00000000
>>>>     %4 = zext i1 %3 to i8
>>>>     store i8 %4, i8* @b, align 1, !tbaa !2
>>>>     ret void
>>>> }
>>>
>>> Exactly. But this behavior is desirable, locally. There's no good answer
>>> here: We either generate extra load traffic here (because we need to load
>>> the fields separately), or we widen the store (which generates extra load
>>> traffic there). Do you know, in terms of performance, which is better in
>>> this case (i.e., is it better to widen the store or split the load)?
>>>
>>>   -Hal
>>>
>>>
>>>> Thanks,
>>>> Wei.
>>>>
>>>>>> The testcases shows the potential problem of store shrinking. Before
>>>>>> we decide to do store shrinking, we need to know all the related loads
>>>>>> will be shrunk,  and that requires IPA analysis. Otherwise, when load
>>>>>> shrinking was blocked for some difficult case (Like the instcombine
>>>>>> case described in
>>>>>> https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg65085.html),
>>>>>> performance regression will happen.
>>>>>>
>>>>>> Wei.
>>>>>>
>>>>>>
>>>>>>>> Repository:
>>>>>>>>      rL LLVM
>>>>>>>>
>>>>>>>> https://reviews.llvm.org/D36562
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>> --
>>>>> Hal Finkel
>>>>> Lead, Compiler Technology and Programming Languages
>>>>> Leadership Computing Facility
>>>>> Argonne National Laboratory
>>>>>
>>> --
>>> Hal Finkel
>>> Lead, Compiler Technology and Programming Languages
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>>
>>
>> --
>> Hal Finkel
>> Lead, Compiler Technology and Programming Languages
>> Leadership Computing Facility
>> Argonne National Laboratory

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory



More information about the cfe-commits mailing list