[PATCH] D30751: [MachineCopyPropagation] Extend pass to do COPY source forwarding
Geoff Berry via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 11 14:33:48 PDT 2017
gberry marked 12 inline comments as done.
gberry added inline comments.
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:405
+ SmallVector<MachineInstr *, 4> DeadInsts;
+ LiveInterval &LI = LIS->getInterval(CopyDstReg);
> Unless I am missing something, I don't see the extend to use call for CopySrcReg.
That was being done before the call to this function. I've moved it into this function in my latest revision.
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:452
+ // restrict the types of instructions we forward physical regs into.
> I would avoid forwarding on physical register period.
I have seen some benefit to forwarding physical registers, mostly in cases where block boundaries are changed between RA time and the second run of this pass. This mostly seems to happen when tail merge/tail duplication changes which uses COPYs are exposed to in the same block. This pass won't remove any physical register writes, so this seems relatively safe, other than the caveat in the comment above.
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:524
+ if (hasImplicitOverlap(MOUse, MI))
> We don't need this check for virtual reg, right?
No, I don't think so. I added an early exit for this case to hasImplicitOverlap.
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:571
> It would help the readability if there were helper function for the different regclass/register fixing for the respective physreg and virtureg cases.
> E.g., a helper with if NewUseReg is a physreg then block line 485 else block line 541.
> Then the code would look like:
> // Check if replacing is possible between MOUse and NewUseReg
> // Adapt NewUseReg to whatever constraints are carried by MOUse
> fix(NewUseReg, NewUseSubReg); // <--- This call your helper function
I've factored out quite a bit of code from this function, let me know what you think.
Comment at: test/CodeGen/AArch64/flags-multiuse.ll:1
-; RUN: llc -mtriple=aarch64-none-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -verify-machineinstrs -o - %s | FileCheck %s
+; RUN: llc -mtriple=aarch64-none-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -disable-post-ra -verify-machineinstrs -o - %s | FileCheck %s
> Why do we need to change the run line here?
Turning off the post-RA scheduler kept the checked instructions in the same order. I've just re-arranged the checks now.
More information about the llvm-commits