[PATCH] D31821: Remove redundant copy in recurrences

[Taewook Oh via Phabricator via llvm-commits]

twoh added a comment.

@MatzeB Thank you for your comments. I'm collection compile time numbers now with spec2006. For our internal benchmark, which takes about ~40min to compile, the compile time difference was negligible.

I don't think this patch can be implemented without loop info, because this is formulated around the loop recurrence pattern. It might be possible but will eventually require more code to produce the information provided by MachineLoopInfo. I'm curious what is your biggest concern about using MachineLoopInfo in this pass, as it is used in other passes as well.

I agree on you that maintainability vs supporting more patterns is a hard trade-off. IMHO, if we want to generate better performing code, adding more code to support more patterns would be unavoidable, unless we re-architecturing register allocation related passes. I think this patch adds more lines of code because this handles the pattern based on the loop structure for the first time.

I tried .mir test, but the output is still not two-address format. I tried

`./bin/llc -mtriple=x86_64-- -run-pass=twoaddressinstruction -o - ./input.mir`

(input.mir is from -stop-before=twoaddressinstruction), and the output still has instructions such as

`%vreg10<def,tied1> = ADD32rr %vreg1<kill,tied0>, %vreg0<kill>, %EFLAGS<imp-def,dead>; GR32:%vreg10,%vreg1,%vreg0`
.



================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:714-715
+    for (auto &Def: MRI->def_instructions(UD->getOperand(1).getReg())) {
+      if (MLI->getLoopFor(Def.getParent()) != Loop)
+        continue;
+
----------------
MatzeB wrote:
> Isn't this check superfluous? I would expect this to be true anyway if the instruction is inside `MBB` which is checked later.
This check is here to filter the definitions that are outside of Loop. The check below is to see if the definition inside the Loop belongs to MBB. 


https://reviews.llvm.org/D31821