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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


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

> If PLDpc already depends on Pred then there is nothing more to do and you probably don't need the artificial edge.
We need to check all the preds of PLDpc which will take up some time. And I think, there is not big problem to have an artificial edge here. 

> If Pred depends on PLDpc then we cannot add a new dependency because that would create a circular dependency.
It has been already checked by addEdge.


================
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()) {
----------------
stefanp wrote:
> I agree. We do need to merge the functionality of  `hasPCRelForm` from here and `hasPCRelativeForm` into one place. (In a different patch though...)
> 
Agree


================
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
----------------
stefanp wrote:
> 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.
Yes, we need some follow up patch to fix this issue as such kind of bug is really hard to address.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87071