Differential D129525
[GlobalOpt] Drop SRA split limit for struct types. Authored by fhahn on Jul 11 2022, 5:26 PM.
Details 4796b4ae7bccc7 limited the SRA to 16 types for all globals, while the This is causing notable size regressions for some large workloads. This
Diff Detail
Unit Tests Event TimelineComment Actions This will also bypass the check if you have an array that is nested inside a struct. Besides, I don't think we should be making decisions based on the global value type here, which is a another vacuous concept that shouldn't exist (e.g. in rust the global type will often be something like [N x i8], in "bag of bytes" representation). Does raising the limit to the next power of two cover your test case? Comment Actions Right , I think this should be similar to the previous cut-off for struct with arrays inside, e.g.: https://llvm.godbolt.org/z/crje3aYzW . I added additional tests with a struct containing an array and a plain array. I agree that the type shouldn't really matter. But I think changing the way the cut-off is handled should be done independently of the change to use an offset-based approach.
Unfortunately the struct has stores to 100+ fields in the workload. Comment Actions @fhahn I think your last update is missing the code changes :) I assume that you don't actually care about SRA as such, but rather about some follow-on transforms it enables. I've been thinking that we might want to make GlobalOpt in general work on the SRAd representation internally, which might remove the need to actually materialize it in some cases. So if some parts of the global are never written, it will certainly always be profitable to replace them with (part of) the initializer, regardless of how many parts there are. The limit is only relevant for cases where we actually do leave behind the globals. Do you know which optimization in particular is important for your workload? Comment Actions I didn't make any code changes to the original version and it looks like the original code change is still there. Am I missing something? :)
Yeah, ideally SRA would not materialize each transform it applies straight away, e.g. for the test cases the split up globals will get removed after materializing them. I think the main regression comes from SRA not removing stores of function pointers that are never read, which in turn prevents LLVM from removing those functions. Comment Actions Oh sorry, I misread your comment. I thought you wanted to add a check for nested arrays, but you were only referring to new tests.
Okay, that's a particularly simple case. It's probably worthwhile to give skipping parts that are only stored a try, as we expect them to be dropped (it's not strictly guaranteed due to leak checker roots, but close enough). I can take a look. Comment Actions Just a rebase on top of a test added in 91e67c074922cc667fa1c43fc1f01acb96faa0f9 that can't be handled by D129857. Comment Actions I've updated D129857 to handle the new case -- I'd still generally prefer not to special case struct types if we can avoid it. Comment Actions We should be able to handle all cases with D129857 + follow-on changes. let's go down that route! |