[PATCH] D26066: [PowerPC] Improvements for BUILD_VECTOR Vol. 4

[Kit Barton via llvm-commits]

kbarton requested changes to this revision.
kbarton added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:323
+          };
+          if (P1 && P2) {
+            removeFRSPIfPossible(P1);
----------------
If I understand this logic correctly, you only want to remove the FRSP if *both* operands are created using it. If one parameter is defined using an FSPR and the other one isn't, then you can't remove it in one case. 
This logic currently won't do that, because the removeFRSPIfPossible lambda looks at the operands independently. So you need to change the lambda into a check if it's safe to remove the FSPR, and if it's safe to do for both P1 and P2, then remove them (or change the logic in the lambda in a different way to ensure equivalent functionality). 


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:325
+            removeFRSPIfPossible(P1);
+            if (P1 != P2)
+              removeFRSPIfPossible(P2);
----------------
Are you sure this will do what you want after P1 has possibly been erasedFromParent?
The documentation for eraseFromParent indicates the node is removed from the instruction list and deleted. I think it's better to do this check before calling removeFRSPIfPossible(P1). 


Repository:
  rL LLVM

https://reviews.llvm.org/D26066