[PATCH] D87071: [Scheduling] Add a mutation to schedule GOT indirect instructions close to each other for linker optimization

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Overall I think that the patch looks good. I just had a couple of comments.
Thank you for finding the issues related to the peephole!



================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:192
+                      << ")\n");
+    DAG.addEdge(&PLDpc, SDep(Pred.getSUnit(), SDep::Artificial));
+  }
----------------
Do we need to check that there isn't already a dependency between Pred and PLDpc?

If `PLDpc` already depends on `Pred` then there is nothing more to do and you probably don't need the artificial edge.
If `Pred` depends on `PLDpc` then we cannot add a new dependency because that would create a circular dependency.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:198
+  // TODO - Using InstrMapping to map the opcode for Pcrel form and non-Pcrel
+  // form.
+  switch (MI.getOpcode()) {
----------------
I agree. We do need to merge the functionality of  `hasPCRelForm` from here and `hasPCRelativeForm` into one place. (In a different patch though...)



================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:272
+
+  // Asume that, the second operand is the base register of the load/store and
+  // it must be the result register of the PLDpc so that, it can be folded into
----------------
nit:
`Assume`


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll:26
 ; CHECK-NEXT:    pstd r4, FuncLocal at PCREL(0), 1
+; CHECK-NEXT:    std r4, 0(r3)
 ; CHECK-NEXT:    blr
----------------
steven.zhang wrote:
> std and pstd is not aliased. We can still do the linker opt for it but now, we can't as peephole didn't know they are not aliased. Further, we didn't emit the symbol because we have uses in-between pld and std which is too conservative also. If the other instr is store, we won't have problems if there is any uses in-between the pair. 
Yes there is room for improvement in the peephole because we do not check aliases at all. This could potentially be a bug if we clobber memory without knowing it. Good catch!
The peephole also does not differentiate between loads and stores. I agree it is safe to have uses if the instruction is a store. However, it is not safe to have uses if the second instruction is a load.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87071