[PATCH] D151069: [AMDGPU] Handle multiple occurences of an incoming value in break large PHIs

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1575
+  VectorSlice(Type *Ty, unsigned Idx, unsigned NumElts)
+      : Ty(Ty), Idx(Idx), NumElts(NumElts) {}
+
----------------
rovka wrote:
> Nit: I would assert that NumElts matches Ty.
It's not really necessary because NumElts doesn't relate to Ty, but to the original vector type. Currently yes, if NumElts == 1 then Ty is scalar, and if NumElts > 1 then Ty if FixedVectorType, but it's really not required. 

e.g. We could even decide to bitcast the slice to another type (e.g. a 4 x i8 slice to a i32) and that'd still be fine (NumElts would be 4 for a scalar Ty, but it's ok because 4 elements of the original vector can be bitcasted to Ty).


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1590
+  ///   the value in bb.1 may not be reachable from bb.0 if it's its
+  ///   predecessor.)
+  ///   - We also want to make our extract instructions as local as possible so
----------------
rovka wrote:
> Maybe add a test for this case, so we don't regress in the future?
`phi_v7i16_switch` already covers it (it's what caught the issue when I was just checking Values without BBs at first)


================
Comment at: llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-break-large-phis-heuristics.ll:15
 ; CHECK-NEXT:    [[LARGEPHI_EXTRACTSLICE0:%.*]] = extractelement <5 x double> [[X]], i64 0
-; CHECK-NEXT:    [[LARGEPHI_EXTRACTSLICE1:%.*]] = extractelement <5 x double> [[X]], i64 1
-; CHECK-NEXT:    [[LARGEPHI_EXTRACTSLICE2:%.*]] = extractelement <5 x double> [[X]], i64 2
-; CHECK-NEXT:    [[LARGEPHI_EXTRACTSLICE3:%.*]] = extractelement <5 x double> [[X]], i64 3
-; CHECK-NEXT:    [[LARGEPHI_EXTRACTSLICE4:%.*]] = extractelement <5 x double> [[X]], i64 4
+; CHECK-NEXT:    [[LARGEPHI_EXTRACTSLICE2:%.*]] = extractelement <5 x double> [[X]], i64 1
+; CHECK-NEXT:    [[LARGEPHI_EXTRACTSLICE4:%.*]] = extractelement <5 x double> [[X]], i64 2
----------------
rovka wrote:
> Nit: Commit the irrelevant test updates (i.e. the variable renamings) separately?
I would have done that if I could, but the naming changes are unfortunately a side-effect of restructuring the code.

I could have tried harder to preserve the old names but IMO it's just debug names that don't really matter, so I thought it'd be fine as is. Is that ok?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151069