[PATCH] D151069: [AMDGPU] Handle multiple occurences of an incoming value in break large PHIs
Pierre van Houtryve via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 22 04:04:51 PDT 2023
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
More information about the llvm-commits
mailing list