[PATCH] D65740: [ARM][ParallelDSP] Replace SExt uses

[Sam Parker via Phabricator via llvm-commits]

samparker marked an inline comment as done.
samparker added a comment.

Because of the VecLd[0]/LHS typo, I had assumed that there was something wrong with the handling of the 'exchange' instructions. When I added that functionality, the tests weren't very varied so I assumed that I had missed an edge case with operand orderings. What the tests show is that there are some patterns which don't get optimised well, see the TODOs. The tests in overlapping are the kind of accesses that cause the problems and it wasn't something I had thought about. We test for sext loads with multiple users, but not in the case where multiple ones are widened.



================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:700
+  Value *NewBaseSExt = IRB.CreateSExt(Bottom, BaseSExt->getType());
+  BaseSExt->replaceAllUsesWith(NewBaseSExt);
 
----------------
efriedma wrote:
> I don't understand how this makes a difference; the new SExt has to have the same operand and result type as the old one, I think, so making a new SExt followed by RAUW should be the same as changing the operand of the original.  Is the old instruction in the wrong position?  Or is there some sort of invalidation issue?
The issue arises when a load is used to create more than one wide load. In the first instance, the sext would use the new load and the original load would have no users. In the second instance, when we enter this method, we'll hit the assertion because we try to look at the sext user of the original load.


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

https://reviews.llvm.org/D65740