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

[Diana Picus via Phabricator via llvm-commits]

rovka 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) {}
+
----------------
Nit: I would assert that NumElts matches 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
----------------
Maybe add a test for this case, so we don't regress in the future?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1595
+  ///
+  /// This is both a minor optimization to avoid creating duplicate instruction,
+  /// but also a requirement for correctness. It is not forbidden for a PHI node
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1599
+  /// each time,
+  ///  those previously identical pairs would all have different incoming values
+  ///  (from the same block) and it'd cause a
----------------
Nit: Reformat the rest of the comment?


================
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
----------------
Nit: Commit the irrelevant test updates (i.e. the variable renamings) separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151069