[llvm] [Delinearization] Add function for fixed size array without relying on GEP (PR #145050)
Sjoerd Meijer via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 8 02:52:53 PDT 2025
sjoerdmeijer wrote:
> > * Shall we rename `delinearizeFixedSizeArray` to `tryDelinearizeFixedSizeImplExperimental`; maybe that makes things more clear/explicit that one is going to replace the other?
>
> I don't have a strong preference for the current name. However, I'd rather avoid using a word like `experimental` in the function name (similarly, I'm not a big fan of intrinsics such as `llvm.experimental.*` either). IMHO, it's preferable to attach that kind of information via an attribute or a comment instead. So my suggestion would be to mark the existing `tryDelinearizeFixedSizeImpl` as deprecated with `[[deprecated]]` attribute. WDYT?
That will work for me, and I don't have very strong opinions in the first place. My only observation was that having both functions is confusing for people who have less context. So if [[deprecated]] is one way to do this, then that sounds good to me.
>
> > * Is there a way we can compare the the new and old delinearization? Or is it too early for that? For example, can we compare the return values where this is used, e.g.:
> > ```
> > bool TrySrcOld = tryDelinearizeFixedSizeImpl(SE, Src, SrcAccessFn, SrcSubscripts, SrcSizes);
> > bool TrySrcNew = tryDelinearizeFixedSizeImplExperimental(SE, Src, SrcAccessFn, SrcSubscripts, SrcSizes);
> > assert(TrySrcOld == TrySrcNew && "something");
> > ```
>
> What are you trying to achieve? As things stand, there are still cases where the old function works but the new one doesn't, so I think adding an assert like that would be too early. If your goal is just to compare the capabilities of the two functions, I think that's possible, but I don’t intend to upstream that kind of code.
Yeah, exactly, I was trying to see if we could systematically catch cases where there's divergence between old and new. But I see that it is early days, and maybe tricky.
>
> > * In [this](https://github.com/llvm/llvm-project/pull/144088#discussion_r2145496052) comment for another change you provided another example where DA is not doing the right thing. Is that case handled here too? Is it worth adding this here, or is it exactly the same as one of the other cases?
>
> I owe you an apology regarding this. I realized the corner case I mentioned was slightly wrong. Sorry about that! (I had used a small script to generate the test case, but it was buggy. I’ve discarded that script immediately). I'll add more details in [the comment you mentioned](https://github.com/llvm/llvm-project/pull/144088#discussion_r2145496052). Anyway, these cases still cause issues in DA, and I believe replacing `tryDelinearizeFixedSizeImpl` with the new function could resolve them. I'm not entirely sure whether it's worth adding a test case, but if so, it would be better to include it in a separate patch.
No worries, and thanks for adding the test case. That's the only thing I wanted to achieve: collect all interesting test cases that were floating around in different merge requests. :)
https://github.com/llvm/llvm-project/pull/145050
More information about the llvm-commits
mailing list