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

[Diana Picus via Phabricator via llvm-commits]

rovka accepted this revision.
rovka added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1575
+  VectorSlice(Type *Ty, unsigned Idx, unsigned NumElts)
+      : Ty(Ty), Idx(Idx), NumElts(NumElts) {}
+
----------------
Pierre-vh wrote:
> 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).
Ok, thanks for explaining. Maybe add that to that slice examples then? (optional)


================
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
----------------
Pierre-vh wrote:
> 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?
Fine either way :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151069