[InstCombine] correct operands of shouldChangeType() for casted phi transform

[spatel]

Summary

This is a bug noted in the recent D72733 and seen in the similar transform just above the changed source code.

I added tests with illegal types and zexts to show the bug - we could transform legal phi ops to illegal, etc. I did not add tests with trunc because we won't see any diffs on those patterns. That is because InstCombiner::SliceUpIllegalIntegerPHI() appears to do those transforms independently of datalayout. It can also create more casts than are present in existing code. I'm not sure if that is intended or not, but that might be another patch.

There are some existing regression tests that do not include a datalayout that would be altered by this fix. I assumed that the lack of a datalayout in those regression files is an oversight, so I added the minimal layout (make i32 legal) necessary to preserve behavior on those tests.

[lebedev.ri]

Looks good.

[mstorsjo]

This causes failed asserts for me. With https://martin.st/temp/substtml-preproc.c, built with clang -target i686-w64-mingw32 -c -O2 substtml-preproc.c, I'm getting this:

clang: ../include/llvm/Support/Alignment.h:384: llvm::Align llvm::operator/(llvm::Align, uint64_t): Assertion `Lhs != 1 && "Can't halve byte alignment"' failed.

[lebedev.ri]

CC'ing Align suspects

In D73907#1865490, @mstorsjo wrote:

This causes failed asserts for me. With https://martin.st/temp/substtml-preproc.c, built with clang -target i686-w64-mingw32 -c -O2 substtml-preproc.c, I'm getting this:

clang: ../include/llvm/Support/Alignment.h:384: llvm::Align llvm::operator/(llvm::Align, uint64_t): Assertion `Lhs != 1 && "Can't halve byte alignment"' failed.

[spatel]

In D73907#1865494, @lebedev.ri wrote:

CC'ing Align suspects

In D73907#1865490, @mstorsjo wrote:

This causes failed asserts for me. With https://martin.st/temp/substtml-preproc.c, built with clang -target i686-w64-mingw32 -c -O2 substtml-preproc.c, I'm getting this:

clang: ../include/llvm/Support/Alignment.h:384: llvm::Align llvm::operator/(llvm::Align, uint64_t): Assertion `Lhs != 1 && "Can't halve byte alignment"' failed.

I'm not seeing a bug in this patch itself. Here's a reduced test that crashes with that assert with only "opt -codegenprepare":

target datalayout = "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:32-n8:16:32-a:0:32-S32"
target triple = "i686-w64-windows-gnu"

define void @ttml_read_coords(float %x, i64* %p) {
  %b = bitcast float %x to i32
  %z = zext i32 0 to i64
  %s = shl nuw nsw i64 %z, 32
  %z2 = zext i32 %b to i64
  %o = or i64 %s, %z2
  store i64 %o, i64* %p, align 1
  ret void
}

I'll try to fix this tomorrow if nobody else gets to it sooner.

[courbet]

"opt -codegenprepare":

I think you meant llc. Thanks for the repro. D73274 is indeed responsible for the crash, but it only exhibited an issue with the existing code: An alignment of 1 was previously silently changed to an alignment of 0. The new typed Align catches this: When splitting a 1-aligned store into two stores, both stores are 1-aligned, instead of one being 1-aligned and the other being 0-aligned (which does not really mean anything since it has different semantics depending on where you are in LLVM)

I'll send a fix.

[spatel]

In D73907#1866565, @courbet wrote:

"opt -codegenprepare":

I think you meant llc.

With CGP, we can use either llc or opt to test, but if the bug is visible/reproducible with opt, then it's better to use that invocation. That's because the regression test can then be an IR --> IR test, and that is (almost?) always simpler to test/understand than IR --> some-other-language.

[spatel]

In D73907#1866565, @courbet wrote:

I'll send a fix.

Thanks! For reference, I think this is same/similar to PR44851 (the test case there is slightly smaller):
https://bugs.llvm.org/show_bug.cgi?id=44851

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

if (auto *Sel = dyn_cast<SelectInst>(Src)) {
     }
   }
 
   // If we are casting a PHI, then fold the cast into the PHI.
   if (auto *PN = dyn_cast<PHINode>(Src)) {
     // Don't do this if it would create a PHI node with an illegal type from a
     // legal type.
     if (!Src->getType()->isIntegerTy() || !CI.getType()->isIntegerTy() ||
-        shouldChangeType(CI.getType(), Src->getType()))
+        shouldChangeType(CI.getSrcTy(), CI.getType()))
       if (Instruction *NV = foldOpIntoPhi(CI, PN))
         return NV;
   }
 
   return nullptr;
 }
 
 /// Constants and extensions/truncates from the destination type are always

llvm/test/CodeGen/AMDGPU/diverge-switch-default.ll

 ; RUN: llc -march=amdgcn -mcpu=gfx900 -print-after=si-annotate-control-flow %s -o /dev/null 2>&1 | FileCheck %s
 
+target datalayout = "n32"
+
 ; CHECK-LABEL: @switch_unreachable_default
 
 define amdgpu_kernel void @switch_unreachable_default(i32 addrspace(1)* %out, i8 addrspace(1)* %in0, i8 addrspace(1)* %in1) #0 {
 centry:
   %tid = call i32 @llvm.amdgcn.workitem.id.x()
   switch i32 %tid, label %sw.default [
     i32 0, label %sw.bb0
     i32 1, label %sw.bb1

llvm/test/CodeGen/AMDGPU/reqd-work-group-size.ll

 ; RUN: opt -mtriple=amdgcn-amd-amdhsa -S -amdgpu-lower-kernel-attributes -instcombine %s | FileCheck -enable-var-scope %s
 
+target datalayout = "n32"
+
 ; CHECK-LABEL: @invalid_reqd_work_group_size(
 ; CHECK: load i16,
 define amdgpu_kernel void @invalid_reqd_work_group_size(i16 addrspace(1)* %out) #0 !reqd_work_group_size !1 {
   %dispatch.ptr = tail call i8 addrspace(4)* @llvm.amdgcn.dispatch.ptr()
   %gep.group.size.x = getelementptr inbounds i8, i8 addrspace(4)* %dispatch.ptr, i64 4
   %gep.group.size.x.bc = bitcast i8 addrspace(4)* %gep.group.size.x to i16 addrspace(4)*
   %group.size.x = load i16, i16 addrspace(4)* %gep.group.size.x.bc, align 4
   store i16 %group.size.x, i16 addrspace(1)* %out

llvm/test/Transforms/InstCombine/cast_phi.ll

 
 define i37 @zext_from_legal_to_illegal_type(i32 %x) {
 ; CHECK-LABEL: @zext_from_legal_to_illegal_type(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[X:%.*]], 42
 ; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
 ; CHECK:       t:
 ; CHECK-NEXT:    [[Y:%.*]] = call i32 @get_i32()
-; CHECK-NEXT:    [[PHITMP:%.*]] = zext i32 [[Y]] to i37
 ; CHECK-NEXT:    br label [[EXIT:%.*]]
 ; CHECK:       f:
 ; CHECK-NEXT:    call void @bar()
 ; CHECK-NEXT:    br label [[EXIT]]
 ; CHECK:       exit:
-; CHECK-NEXT:    [[P:%.*]] = phi i37 [ [[PHITMP]], [[T]] ], [ 3, [[F]] ]
-; CHECK-NEXT:    ret i37 [[P]]
+; CHECK-NEXT:    [[P:%.*]] = phi i32 [ [[Y]], [[T]] ], [ 3, [[F]] ]
+; CHECK-NEXT:    [[R:%.*]] = zext i32 [[P]] to i37
+; CHECK-NEXT:    ret i37 [[R]]
 ;
 entry:
   %cmp = icmp eq i32 %x, 42
   br i1 %cmp, label %t, label %f
 
 t:
   %y = call i32 @get_i32()
   br label %exit

 
 define i37 @zext_from_illegal_to_illegal_type(i32 %x) {
 ; CHECK-LABEL: @zext_from_illegal_to_illegal_type(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[X:%.*]], 42
 ; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
 ; CHECK:       t:
 ; CHECK-NEXT:    [[Y:%.*]] = call i3 @get_i3()
-; CHECK-NEXT:    [[PHITMP:%.*]] = zext i3 [[Y]] to i37
 ; CHECK-NEXT:    br label [[EXIT:%.*]]
 ; CHECK:       f:
 ; CHECK-NEXT:    call void @bar()
 ; CHECK-NEXT:    br label [[EXIT]]
 ; CHECK:       exit:
-; CHECK-NEXT:    [[P:%.*]] = phi i37 [ [[PHITMP]], [[T]] ], [ 3, [[F]] ]
-; CHECK-NEXT:    ret i37 [[P]]
+; CHECK-NEXT:    [[P:%.*]] = phi i3 [ [[Y]], [[T]] ], [ 3, [[F]] ]
+; CHECK-NEXT:    [[R:%.*]] = zext i3 [[P]] to i37
+; CHECK-NEXT:    ret i37 [[R]]
 ;
 entry:
   %cmp = icmp eq i32 %x, 42
   br i1 %cmp, label %t, label %f
 
 t:
   %y = call i3 @get_i3()
   br label %exit

 
 define i64 @zext_from_illegal_to_legal_type(i32 %x) {
 ; CHECK-LABEL: @zext_from_illegal_to_legal_type(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[X:%.*]], 42
 ; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
 ; CHECK:       t:
 ; CHECK-NEXT:    [[Y:%.*]] = call i3 @get_i3()
+; CHECK-NEXT:    [[PHITMP:%.*]] = zext i3 [[Y]] to i64
 ; CHECK-NEXT:    br label [[EXIT:%.*]]
 ; CHECK:       f:
 ; CHECK-NEXT:    call void @bar()
 ; CHECK-NEXT:    br label [[EXIT]]
 ; CHECK:       exit:
-; CHECK-NEXT:    [[P:%.*]] = phi i3 [ [[Y]], [[T]] ], [ 3, [[F]] ]
-; CHECK-NEXT:    [[R:%.*]] = zext i3 [[P]] to i64
-; CHECK-NEXT:    ret i64 [[R]]
+; CHECK-NEXT:    [[P:%.*]] = phi i64 [ [[PHITMP]], [[T]] ], [ 3, [[F]] ]
+; CHECK-NEXT:    ret i64 [[P]]
 ;
 entry:
   %cmp = icmp eq i32 %x, 42
   br i1 %cmp, label %t, label %f
 
 t:
   %y = call i3 @get_i3()
   br label %exit

llvm/test/Transforms/InstCombine/icmp-div-constant.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
+target datalayout = "n32"
+
 define i1 @is_rem2_neg_i8(i8 %x) {
 ; CHECK-LABEL: @is_rem2_neg_i8(
 ; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[X:%.*]], -127
 ; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[TMP1]], -127
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %s = srem i8 %x, 2
   %r = icmp slt i8 %s, 0

llvm/test/Transforms/InstCombine/icmp-mul-zext.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
+target datalayout = "n32"
+
 define i32 @sterix(i32, i8, i64) {
 ; CHECK-LABEL: @sterix(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CONV:%.*]] = zext i32 [[TMP0:%.*]] to i64
 ; CHECK-NEXT:    [[CONV1:%.*]] = sext i8 [[TMP1:%.*]] to i32
 ; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[CONV1]], 1945964878
 ; CHECK-NEXT:    [[SH_PROM:%.*]] = trunc i64 [[TMP2:%.*]] to i32
 ; CHECK-NEXT:    [[SHR:%.*]] = lshr i32 [[MUL]], [[SH_PROM]]