[PATCH] D4351: bug fix for PR20020: anti-dependency-breaker causes miscompilation

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/CriticalAntiDepBreaker.cpp:203
 
+    // If this reg is tied and live (Classes[Reg] is set to -1), we can't change
+    // it or any of its sub or super regs. We need to use KeepRegs to mark the
----------------
danilaml wrote:
> @spatel Hi, I was debugging some miscompile issue and came across what I think is very similar bug.
> Do you remember why you check against `-1` here? In my test-case (which I'm trying to reduce), the Classes[Reg] is some regclass non -1 value. According to the comments for Classes:
> ```
> /// For live regs that are only used in one register class in a
>     /// live range, the register class. If the register is not live, the
>     /// corresponding value is null. If the register is live but used in
>     /// multiple register classes, the corresponding value is -1 casted to a
>     /// pointer.
> ```
> My question is: why is this code is checking for `-1` specifically, instead of `!= nullptr`? If it's not live, would it still be OK? I have trouble following the logic in this pass.
> The comments for scan indicate that
> ```
> // Note: AntiDepReg may be referenced by a two-address instruction such that
> // it's use operand is tied to a def operand ...
> // In this case, both the use and def operands are in
> // RegRefs because the def is inserted by PrescanInstruction and not erased
> // during ScanInstruction.
> ```
> which is evidently not true in my test case (e.g. XOR has 3 same-reg operands, one pair tied, so the ScanInstruction would remove all RegRefs added by Prescan due to the 3rd non-tied reg, and skip the first tied def when adding refs back).
Wow - I barely remember creating this patch. I haven't looked at anything near here in so long, that I can't offer much help. I see that you posted D107582 - let's see if we can find someone more knowledgeable to review.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D4351