[SelectionDAG] reset NewNodesMustHaveLegalTypes flag between basic blocks

[guyblank]

Summary

The NewNodesMustHaveLegalTypes flag is set to false at the beginning of CodeGenAndEmitDAG, and set to true after legalizing types.
But before calling CodeGenAndEmitDAG we build the DAG for the basic block. So for the first basic block NewNodesMustHaveLegalTypes would be 'false' during the SDAG building, and for all other basic blocks it would be 'true'.

This patch sets the flag to false before SDAG building each basic block.

While the patch affects an AArch64 test, the test would produce the same result in trunk if moved to be the first function in the file. So someone familiar with the AArch64 codegen might want to take a look at the test regardless.

[RKSimon]

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

639 ↗

(On Diff #99860)

Move to the top and comment this properly.

[RKSimon]

Any AARCH64 people want to check the test?

[guyblank]

ping

[RKSimon]

I'm happy with this change but an AARCH64 specialist should review the test's new codegen

[guyblank]

@ahatanak
seeing that you're doing AArch64 codegen commits, can you please take a look at this?

[ahatanak]

I think what this patch is doing is correct, but it seems to me that the generated code got worse.

It looks like AArch64TargetLowering::LowerBUILD_VECTOR turns vector (0,0,-1,0,0,0,0,0) into a constant pool load instead of using immediate moves because the vector type is v8i16, not v8i8. This probably didn't happen previously because NewNodesMustHaveLegalTypes was true during legalization of the second function.

I don't know the right way to fix it (maybe you can ask @t.p.northover?).

[guyblank]

@t.p.northover
does committing this patch and opening a PR for the AArch64 Isel issue sound reasonable?

[guyblank]

ping

[RKSimon]

@t.p.northover Are you happy for the AArch64 codegen regression to be raised as a bug?

[zvi]

@t.p.northover, being listed as AArch64's code owner, can you please assign a person to review this patch? Thanks.

[RKSimon]

@t.p.northover @fhahn Any comments?

[t.p.northover]

I'm really sorry this took so long. This patch is obviously an improvement. A PR against AArch64 would be sort of nice, but I don't think any more is needed because the actual chances of it affecting real codegen seems pretty small (significant vector code in the entry block?).

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

if (ShouldAbort)
     report_fatal_error(R.getMsg());
 
   ORE.emit(R);
 }
 
 void SelectionDAGISel::SelectBasicBlock(BasicBlock::const_iterator Begin,
                                         BasicBlock::const_iterator End,
                                         bool &HadTailCall) {
+  // Allow creating illegal types during DAG building for the basic block.
+  CurDAG->NewNodesMustHaveLegalTypes = false;
+
   // Lower the instructions. If a call is emitted as a tail call, cease emitting
   // nodes for this block.
   for (BasicBlock::const_iterator I = Begin; I != End && !SDB->HasTailCall; ++I) {
     if (!ElidedArgCopyInstrs.count(&*I))
       SDB->visit(*I);
   }
 
   // Make sure the root of the DAG is up-to-date.

llvm/trunk/test/CodeGen/AArch64/fold-constants.ll

entry:
   %vset_lane = insertelement <4 x i16> undef, i16 %vgetq_lane285, i32 0
   %4 = bitcast <4 x i16> %vset_lane to <1 x i64>
   %vget_lane = extractelement <1 x i64> %4, i32 0
   ret i64 %vget_lane
 }
 
 ; PR25763 - folding constant vector comparisons with sign-extended result
 define <8 x i16> @dotests_458() {
+; CHECK-LABEL: .LCPI1_0:
+; CHECK:       .hword  0                       // 0x0
+; CHECK-NEXT:  .hword  0                       // 0x0
+; CHECK-NEXT:  .hword  65535                   // 0xffff
+; CHECK-NEXT:  .hword  0                       // 0x0
+; CHECK-NEXT:  .hword  0                       // 0x0
+; CHECK-NEXT:  .hword  0                       // 0x0
+; CHECK-NEXT:  .hword  0                       // 0x0
+; CHECK-NEXT:  .hword  0                       // 0x0
+
 ; CHECK-LABEL: dotests_458
-; CHECK:       movi d0, #0x00000000ff0000
-; CHECK-NEXT:  sshll v0.8h, v0.8b, #0
+; CHECK:       adrp    x8, .LCPI1_0
+; CHECK-NEXT:  ldr     q0, [x8, :lo12:.LCPI1_0]
 ; CHECK-NEXT:  ret
 entry:
   %vclz_v.i = call <8 x i8> @llvm.ctlz.v8i8(<8 x i8> <i8 127, i8 38, i8 -1, i8 -128, i8 127, i8 0, i8 0, i8 0>, i1 false) #6
   %vsra_n = lshr <8 x i8> %vclz_v.i, <i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5>
   %name_6 = or <8 x i8> %vsra_n, <i8 127, i8 -128, i8 -1, i8 67, i8 84, i8 127, i8 -1, i8 0>
   %cmp.i603 = icmp slt <8 x i8> %name_6, <i8 -57, i8 -128, i8 127, i8 -128, i8 -1, i8 0, i8 -1, i8 -1>
   %vmovl.i4.i = sext <8 x i1> %cmp.i603 to <8 x i16>
   ret <8 x i16> %vmovl.i4.i
 }
 declare <8 x i8> @llvm.ctlz.v8i8(<8 x i8>, i1)