[PATCH] D20102: [ScheduleDAG] Make sure to process all def operands before any use operands

Matthias Braun via llvm-commits llvm-commits at lists.llvm.org
Tue May 10 11:46:45 PDT 2016


I am fine with the change as is now after realizing that it does not change the behaviour of the subregister def case. So this change is fine!

I am still confused as to why we could get away with ignoring those uses before. I guess output dependencies are enough to get the instruction order right, but this should be commented on and I'd like to check that it doesn't influence register pressure computation.

- Matthias

> On May 10, 2016, at 11:40 AM, Andrew Trick via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> atrick added inline comments.
> 
> ================
> Comment at: llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp:974-975
> @@ +973,4 @@
> +      const MachineOperand &MO = MI->getOperand(j);
> +      if (!MO.isReg() || !MO.isUse())
> +        continue;
> +      unsigned Reg = MO.getReg();
> ----------------
> MatzeB wrote:
>> MatzeB wrote:
>>> MatzeB wrote:
>>>> atrick wrote:
>>>>> MatzeB wrote:
>>>>>> This will subregister defs which are uses as well, should be `!MO.readsReg())!
>>>>> Matthias, can you clarify? We don't want addPhysRefDefs to be called twice for the same subreg!
>>>> Something like this:
>>>> `%vreg0:sub0 = ...` is both a definition and a use! (at least if lanemask tracking is not enabled), we still need to record it as a use. physregs never have subregister indexes so MO.readsReg() will never return true for physreg definitions.
>>> It would also be nice to split addPhysRegDeps() into addPhysRegUseDeps() and addPhysRegDefDeps().
>> Hmm it looks like the code before this patch only used output dependencies for this case. So this change is fine in preserving that behavior.
> I *think* Matthias is saying that this patch will fail to call addVRegUseDeps for subregs. So, that needs to be fixed but in a way that doesn't call addPhysRegDeps twice on the same operand.
> 
> Maybe Matthias can suggest a fix!
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D20102
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits



More information about the llvm-commits mailing list