[PATCH] D30081: [PPC] Eliminate more compare instructions using record-form operation

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

The extension to the zero-extending opcodes and to comparison to 1/-1 (which are just "or-equal" comparisons against zero) seems perfectly fine to me. My concern is with the safety of optimizing the situation where the machine instruction feeding the compare resides in a different basic block than the compare. We are effectively hoisting the def of the CR field to a predecessor block, which would be fine if we add the respective vreg copies, etc. But the code that is added does not appear to do that. So what I'd like to see in the code (or comment) is how this issue is addressed.



================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1633
   // Scan forward to find the first use of the compare.
+  bool FoundUse = false;
   for (MachineBasicBlock::iterator EL = CmpInstr.getParent()->end(); I != EL;
----------------
This change doesn't seem meaningful but it's harmless, so I guess it's OK.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1664
   else if (MI->getParent() != CmpInstr.getParent() || Value != 0) {
+    if (FoundUse && !equalityOnly) {
+      MachineInstr *UseMI = &*I;
----------------
It isn't clear from this code how it addresses the concern in the comment. Besides, the comment should be updated if this code alleviates the cross-call concern.
Of course, I haven't checked all the code in this function but this code does not appear to do anything to address a situation where there might be a call between the instruction that will be converted to record form and the actual use of the CR field.


https://reviews.llvm.org/D30081