Bug noted in D112717 can be sidestepped with this change.
Expanding all ConstantExpr involved with LDS up front makes the variable specialisation simpler. Excludes ConstantExpr that don't access LDS to avoid disturbing codegen elsewhere.
Differential D133422
[amdgpu] Expand all ConstantExpr users of LDS variables in instructions JonChesterfield on Sep 7 2022, 6:44 AM. Authored by
Details Bug noted in D112717 can be sidestepped with this change. Expanding all ConstantExpr involved with LDS up front makes the variable specialisation simpler. Excludes ConstantExpr that don't access LDS to avoid disturbing codegen elsewhere.
Diff Detail
Event TimelineComment Actions Haven't landed this previously because of the test churn. check-openmp is fine, check-llvm needs manual patch up. Failed Tests (9): LLVM :: CodeGen/AMDGPU/ds_read2.ll LLVM :: CodeGen/AMDGPU/ds_write2.ll LLVM :: CodeGen/AMDGPU/loop_break.ll LLVM :: CodeGen/AMDGPU/lower-kernel-lds-constexpr.ll LLVM :: CodeGen/AMDGPU/lower-kernel-lds-super-align.ll LLVM :: CodeGen/AMDGPU/lower-lds-struct-aa-memcpy.ll LLVM :: CodeGen/AMDGPU/lower-lds-struct-aa-merge.ll LLVM :: CodeGen/AMDGPU/lower-lds-struct-aa.ll LLVM :: CodeGen/AMDGPU/lower-module-lds-constantexpr.ll
Comment Actions The current revision will fix the O0 codegen bug @carlo.bertolli is looking at. It's possible, though awkward, to keep the original idea of deleting the call to replaceConstantUsesInFunction if the lambda passed to replaceLDSVariablesWithStruct works much harder to identify the function corresponding to a Use. That's approximately as complicated as expanding the ConstantExpr up front. Choosing this approach because it's convenient for the other WIP on LDS which replaces some expressions with function calls and thus also needs to expand ConstantExpr.
Comment Actions
Can you add a regression test for that bug? Comment Actions I could - would have to check the exact failure mode and write a fairly complicated test case. This code can't fall victim to the same bug, as it emits instructions for each phi constantexpr, and the currently broken code is best deleted if/after this patch lands. I'm not sure the test would be value adding but will write it if necessary.
Comment Actions Test case was easier than anticipated - the constantexpr didn't have to be nested and block order doesn't make a difference. Added before commit. Fails the verifier before this patch, correct after.
|
What is replaceConstantExprInFunction?