[PATCH] D79864: [PowerPC] Add new linker optimization for PowerPC

[Stefan Pintilie via Phabricator via llvm-commits]

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

In D79864#2061105 <https://reviews.llvm.org/D79864#2061105>, @efriedma wrote:

> Just looked at what I wrote, and the issue I'm seeing isn't clear.
>
> The code doesn't require that the pld and the lwa are adjacent.  Therefore, it's possible that some instruction between the pld and the lwa writes to the destination register of the lwa.  If there is such an instruction, the linker optimization would cause a miscompile: the loaded value would be clobbered.


You are correct I have not checked for that. I will have to think about how to resolve the issue because I add the linker opt flag to the instruction before register allocation so I can't just check that the registers are the same at that time.
In the meantime, I will fixup the feedback from the other comments.

I could re-verify the instruction after register allocation in another peephole perhaps...



================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:401
+        // optimization.
+        if (!hasPCRelativeForm(Use))
+          break;
----------------
efriedma wrote:
> I think the hasPCRelativeForm check subsumes a bunch of the other checks: for example, if an instruction has one of the listed opcodes, it can't be a call.
You are correct, the previous checks are redundant. I'll remove all four of the previous checks. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:405
+        // The use of this register must be as part of the address and not
+        // part of operand zero of a store.
+        if (ResultOp == Use.getOperand(0).getReg())
----------------
amyk wrote:
> Would it be good to check if it is indeed a store instruction? Something like `Use.mayStore()`, though I realize it might be a bit redundant if we reached this part of the code.
That's a good question. I don't think we need to check if this is a store because:
1) The check `hasPCRelativeForm` limits the valid instructions for this. 
AND
2) If this is a load from the valid instruction pool then operand zero is not a use (And we know that `Use` is a use.)


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

https://reviews.llvm.org/D79864