[PATCH] D80364: [amdgpu] Teach load widening to handle non-DWORD aligned loads.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D80364#2061226 <https://reviews.llvm.org/D80364#2061226>, @hliao wrote:

> In D80364#2061176 <https://reviews.llvm.org/D80364#2061176>, @arsenm wrote:
>
> > In D80364#2060318 <https://reviews.llvm.org/D80364#2060318>, @hliao wrote:
> >
> > > In D80364#2058794 <https://reviews.llvm.org/D80364#2058794>, @arsenm wrote:
> > >
> > > > I'd still like to find a way to avoid a whole extra pass run for this. In the test here, the LoadStoreVectorizer should have vectorized these? Why didn't it?
> > >
> > >
> > >
> > >
> > > In D80364#2058794 <https://reviews.llvm.org/D80364#2058794>, @arsenm wrote:
> > >
> > > > I'd still like to find a way to avoid a whole extra pass run for this. In the test here, the LoadStoreVectorizer should have vectorized these? Why didn't it?
> > >
> > >
> > > That's due to the misaligned load after coalescing. This example is written intentionally to skip LSV and verify that common widened load could be CSEd within DAG. In practice, there won't be such input as the 1st i16 load should be properly annotated with align 4.
> >
> >
> > But the load isn't misaligned. The point of adding the alignment to the intrinsic declaration was so that the whole optimization pipeline would know about the alignment. Taking this example:
> >
> > after opt -instcombine:
>
>
> but we run `llc` only in this test and skip all middle-end optimizations. That's why I said this test is impractical in the normal scenario where middle-end is always run. This test is added to address your previous concern on CSE of widened loads. If not required, I could remove this test.


This was more of a concern when letting the DAG handle it. Since you added the implied align attribute, I don't think anything else should be needed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80364