[PATCH] D59973: [SLP] Refactoring of the operand reordering code.

Wed Apr 3 16:00:44 PDT 2019

vporpo added a comment.

I am not sure how to proceed with the operandorder.ll test. @ABataev do you know we would give higher priority to broadcasts?

================
Comment at: test/Transforms/SLPVectorizer/X86/crash_smallpt.ll:35
+; CHECK-NEXT:    [[TMP1:%.*]] = insertelement <2 x double> [[TMP0]], double 0xBFA5CC2D1960285F, i32 1
+; CHECK-NEXT:    [[TMP2:%.*]] = fadd <2 x double> [[TMP1]], <double 0.000000e+00, double undef>
 ; CHECK-NEXT:    [[TMP3:%.*]] = fmul <2 x double> [[TMP2]], <double 1.400000e+02, double 1.400000e+02>
----------------
RKSimon wrote:
> This looks like a regression - we're losing an all constant operand here (plus the TMP1 no longer simplifies to a single scalar load).
Good catch.
The reason was that we were skipping undefs when trying to match an instruction in getBestOperand().
I updated the code to accept Undefs as a secondary option.
In a later patch we should also add a new ReorderingMode for Undefs.

================
Comment at: test/Transforms/SLPVectorizer/X86/operandorder.ll:47
 ; CHECK-NEXT:    store <2 x double> [[TMP4]], <2 x double>* [[TMP5]], align 4
 ; CHECK-NEXT:    br i1 undef, label [[LP]], label [[EXT:%.*]]
 ; CHECK:       ext:
----------------
RKSimon wrote:
> Hmm - these tests are called 'preserve_broadcast' and we're not preserving the broadcasts any more.... This might not be a bad thing but it'd be good to confirm (and possibly rename the tests if everything is good)
I am not sure why we would prefer to build a broadcast instead of vectorizing the loads.
@ABataev any idea about this?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59973/new/

https://reviews.llvm.org/D59973