HomePhabricator

[MIR] Add simple PRE pass to MachineCSE

Description

[MIR] Add simple PRE pass to MachineCSE

This is the second part of the commit fixing PR38917 (hoisting
partitially redundant machine instruction). Most of PRE (partitial
redundancy elimination) and CSE work is done on LLVM IR, but some of
redundancy arises during DAG legalization. Machine CSE is not enough
to deal with it. This simple PRE implementation works a little bit
intricately: it passes before CSE, looking for partitial redundancy
and transforming it to fully redundancy, anticipating that the next
CSE step will eliminate this created redundancy. If CSE doesn't
eliminate this, than created instruction will remain dead and eliminated
later by Remove Dead Machine Instructions pass.

The third part of the commit is supposed to refactor MachineCSE,
to make it more clear and to merge MachinePRE with MachineCSE,
so one need no rely on further Remove Dead pass to clear instrs
not eliminated by CSE.

First step: https://reviews.llvm.org/D54839

Fixes llvm.org/PR38917

This is fixed recommit of r361356 after PowerPC64 multistage build failure.

Details

Committed
anton-afanasyevJun 9 2019, 5:15 AM
Parents
rL362900: [CaptureTracking] Don't let comparisons against null escape inbounds pointers
Branches
Unknown
Tags
Unknown

Event Timeline

FYI, we're seeing test failures due to this revision. Currently trying to isolate a testcase, but it may be challenging to minimize if the failures are due to a miscompile.

FYI, we're seeing test failures due to this revision. Currently trying to isolate a testcase, but it may be challenging to minimize if the failures are due to a miscompile.

Hi @asbirlea , could you please provide me with details? Where are the failures? What is arch?
Also, I've just pushed subsequent fix following this commit: https://reviews.llvm.org/rL363164. Could it fix the test failures you are seeing?

@asbirlea Btw, this fix was commited for the issue noted here: https://reviews.llvm.org/D56772#1537772

@anton-afanasyev:
The test failures occur in Halide generated code, with the HVX backend. They are not public.
r363164 does not fix the tests; it does change an erroneous output to another erroneous one.

@anton-afanasyev:
The test failures occur in Halide generated code, with the HVX backend. They are not public.
r363164 does not fix the tests; it does change an erroneous output to another erroneous one.

@asbirlea Ok, so I'd like to see a testcase, maybe reduced one. It could be the case my commit triggers any hidden arch-specific bug. For instance, now MachineCSE pass may duplicate instructions, so for instrs which cannot be duplicated special MIR flag should be set.

Also, you are talking the erroneous output changed after r363164 -- this likely means test case contains exception handling. I believe this commit can trigger EH bugs, but still need detailed testcase.

@anton-afanasyev: FYI, we're still seeing this and it presents itself as a miscompile.
We're still working on getting a test case but it's proving to be very difficult.

A change that resolves the problem is restricting reachability in a single direction.

...
  ((isPotentiallyReachable(BB1, BB) && !isPotentiallyReachable(BB, BB1)) ||
   (isPotentiallyReachable(BB, BB1) && !isPotentiallyReachable(BB1, BB))))
...

However I am not clear why this would be a needed condition, so it is possible this is only hiding the cause.

@anton-afanasyev: FYI, we're still seeing this and it presents itself as a miscompile.
We're still working on getting a test case but it's proving to be very difficult.

Hi @asbirlea, thanks, please keep me informed about this.

A change that resolves the problem is restricting reachability in a single direction.

...
  ((isPotentiallyReachable(BB1, BB) && !isPotentiallyReachable(BB, BB1)) ||
   (isPotentiallyReachable(BB, BB1) && !isPotentiallyReachable(BB1, BB))))
...

However I am not clear why this would be a needed condition, so it is possible this is only hiding the cause.

I do not see why this condition is needed.

Hi Anton,

Attached is an mir test for a reproducer to a bug that we encountered that appears to be an incorrect behavior of MachineCSE pass after this change was committed.
Can you please take a look ASAP?

You can run it using the following command:
llc -mtriple=aarch64-none-linux-gnu -run-pass=machine-cse -asm-verbose=1 -verify-machineinstrs reproducer.mir -o after-pass
You can see that the divide instruction was pulled out of the if-if-else and it can divide by 0 when the original code doesn't allow this.

Thanks,
Ayman Musa

aymanmus added a subscriber: igorb.Wed, Jun 26, 2:08 AM

Hi Anton,

Attached is an mir test for a reproducer to a bug that we encountered that appears to be an incorrect behavior of MachineCSE pass after this change was committed.
Can you please take a look ASAP?

You can run it using the following command:
llc -mtriple=aarch64-none-linux-gnu -run-pass=machine-cse -asm-verbose=1 -verify-machineinstrs reproducer.mir -o after-pass
You can see that the divide instruction was pulled out of the if-if-else and it can divide by 0 when the original code doesn't allow this.

Thanks,
Ayman Musa

Hi Ayman,
Yes, I'm looking into it now. Looks like a really bug, I'm to figure out where this bug is actually hidden.