<div dir="ltr">LGTM</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 16, 2016 at 3:56 PM, Guozhi Wei <span dir="ltr"><<a href="mailto:carrot@google.com" target="_blank">carrot@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Carrot updated this revision to Diff 50887.<br>
Carrot added a comment.<br>
<br>
This updated patch fixed <a href="https://llvm.org/bugs/show_bug.cgi?id=26918" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=26918</a>.<br>
<br>
The problem of the reverted patch is when we have following code<br>
<br>
; <label>:2:                                      ; preds = %0<br>
<br>
  %3 = load atomic i64, i64* %1 monotonic, align 8<br>
  br label %8<br>
<br>
; <label>:4:                                      ; preds = %0, %0<br>
<br>
  %5 = load atomic i64, i64* %1 acquire, align 8<br>
  br label %8<br>
<br>
; <label>:6:                                      ; preds = %0<br>
<br>
  %7 = load atomic i64, i64* %1 seq_cst, align 8<br>
  br label %8<br>
<br>
; <label>:8:                                      ; preds = %6, %4, %2<br>
<br>
  %.in1097491 = phi i64 [ %3, %2 ], [ %7, %6 ], [ %5, %4 ]<br>
  %9 = bitcast i64 %.in1097491 to double<br>
  ret double %9<br>
<br>
My patch changed it to<br>
<br>
; <label>:2:                                      ; preds = %0<br>
<br>
  %3 = load atomic i64, i64* %1 monotonic, align 8<br>
  %4 = bitcast i64 %3 to double<br>
  br label %11<br>
<br>
; <label>:5:                                      ; preds = %0, %0<br>
<br>
  %6 = load atomic i64, i64* %1 acquire, align 8<br>
  %7 = bitcast i64 %6 to double<br>
  br label %11<br>
<br>
; <label>:8:                                      ; preds = %0<br>
<br>
  %9 = load atomic i64, i64* %1 seq_cst, align 8<br>
  %10 = bitcast i64 %9 to double<br>
  br label %11<br>
<br>
; <label>:11:                                     ; preds = %8, %5, %2<br>
<br>
  %12 = phi double [ %4, %2 ], [ %10, %8 ], [ %7, %5 ]<br>
  %.in1097491 = phi i64 [ %3, %2 ], [ %9, %8 ], [ %6, %5 ]<br>
  ret double %12<br>
<br>
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.<br>
<br>
There are two changes compared to the original approved patch:<br>
<br>
  1 avoid changing volatile/atomic memory access<br>
  2 add the impacted instructions to combining worklist<br>
<br>
Please take another look.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D14596" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14596</a><br>
<br>
Files:<br>
  lib/Transforms/InstCombine/InstCombineCasts.cpp<br>
  lib/Transforms/InstCombine/InstCombineInternal.h<br>
  test/Transforms/InstCombine/pr25342.ll<br>
<br>
</div></div></blockquote></div><br></div>