[PATCH] D14596: [SROA] Choose more profitable type in findCommonType

[David Majnemer via llvm-commits]

LGTM

On Wed, Mar 16, 2016 at 3:56 PM, Guozhi Wei <carrot at google.com> wrote:

> Carrot updated this revision to Diff 50887.
> Carrot added a comment.
>
> This updated patch fixed https://llvm.org/bugs/show_bug.cgi?id=26918.
>
> The problem of the reverted patch is when we have following code
>
> ; <label>:2:                                      ; preds = %0
>
>   %3 = load atomic i64, i64* %1 monotonic, align 8
>   br label %8
>
> ; <label>:4:                                      ; preds = %0, %0
>
>   %5 = load atomic i64, i64* %1 acquire, align 8
>   br label %8
>
> ; <label>:6:                                      ; preds = %0
>
>   %7 = load atomic i64, i64* %1 seq_cst, align 8
>   br label %8
>
> ; <label>:8:                                      ; preds = %6, %4, %2
>
>   %.in1097491 = phi i64 [ %3, %2 ], [ %7, %6 ], [ %5, %4 ]
>   %9 = bitcast i64 %.in1097491 to double
>   ret double %9
>
> My patch changed it to
>
> ; <label>:2:                                      ; preds = %0
>
>   %3 = load atomic i64, i64* %1 monotonic, align 8
>   %4 = bitcast i64 %3 to double
>   br label %11
>
> ; <label>:5:                                      ; preds = %0, %0
>
>   %6 = load atomic i64, i64* %1 acquire, align 8
>   %7 = bitcast i64 %6 to double
>   br label %11
>
> ; <label>:8:                                      ; preds = %0
>
>   %9 = load atomic i64, i64* %1 seq_cst, align 8
>   %10 = bitcast i64 %9 to double
>   br label %11
>
> ; <label>:11:                                     ; preds = %8, %5, %2
>
>   %12 = phi double [ %4, %2 ], [ %10, %8 ], [ %7, %5 ]
>   %.in1097491 = phi i64 [ %3, %2 ], [ %9, %8 ], [ %6, %5 ]
>   ret double %12
>
> And expects ld/st combining can combine the load instructions with
> following bitcast. But ld/st combining doesn't change volatile/atomic
> memory access, so the bitcast instructions are left there. Later phi
> combining transformed it back to the original form, so the two combining
> transforms the code back and forth indefinitely.
>
> There are two changes compared to the original approved patch:
>
>   1 avoid changing volatile/atomic memory access
>   2 add the impacted instructions to combining worklist
>
> Please take another look.
>
>
> http://reviews.llvm.org/D14596
>
> Files:
>   lib/Transforms/InstCombine/InstCombineCasts.cpp
>   lib/Transforms/InstCombine/InstCombineInternal.h
>   test/Transforms/InstCombine/pr25342.ll
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160316/d7e3500f/attachment-0001.html>