[PATCH] D71470: Recommit "[ARM][MVE] findVCMPToFoldIntoVPS"

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer marked 2 inline comments as done.
SjoerdMeijer added a comment.

In D71470#1860178 <https://reviews.llvm.org/D71470#1860178>, @dmgreen wrote:

> Sorry for the delay. The original reproducer I had was with a predicated vdup intrinsic. But when they were added recently it turns out it needs the downstream version (or an odd combination of downstream and upstream intrinsics), and so the same problem didn't show up.
>
> It turns out it was simpler to just write it by hand. Now in as rGf64b3466b6bb <https://reviews.llvm.org/rGf64b3466b6bbea0422209ecaceecd361bb09ff87>.


Thanks, I will look into that.



================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:146
-    --CmpMI;
-    if (CmpMI->modifiesRegister(ARM::VPR, TRI))
-      break;
----------------
dmgreen wrote:
> What happened to this modifies register check?
> 
> This was the original error I was looking at.
I think thhat is covered by `RDA->getReachingMIDef()`, which gets the first def of VPR, and looks to me to be equivalent to the `modifiesRegister()` here if I'm not wrong.
But will take a look at the test case and how that behaves.


================
Comment at: llvm/test/CodeGen/ARM/O3-pipeline.ll:147
 ; CHECK-NEXT:      If Converter
+; CHECK-NEXT:      ReachingDefAnalysis
 ; CHECK-NEXT:      MVE VPT block insertion pass
----------------
dmgreen wrote:
> RDA looks like quite an expensive pass to me. It would scan through and store reaching defs info for every instruction in every block of a function?
> 
> Do you think it's worth it for this case, where we are only doing some minor code cleanup?
There's clearly a trade-off here. The 3 calls to RDA are concise, easy to read, and reusing exiting code. This is a big benefit, but indeed comes at the cost of running RDA. I guess the only way to answer your question is by measuring compile times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71470