Skip to content

[RISC-V] Use of %0 does not have a corresponding definition on every path with -O0 #93587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
patrick-rivos opened this issue May 28, 2024 · 4 comments

Comments

@patrick-rivos
Copy link
Contributor

patrick-rivos commented May 28, 2024

Reduced LLVM IR:

define i16 @f() {
BB:
  br label %BB1

BB1:                                              ; preds = %BB1, %BB
  %A = or i16 0, 0
  %B = fcmp true float 0.000000e+00, 0.000000e+00
  %C = or i1 %B, false
  br i1 %C, label %BB1, label %BB2

BB2:                                              ; preds = %BB1
  ret i16 %A
}

Godbolt: https://godbolt.org/z/7GGzqs5c6

BB state before the LiveIntervalAnalysis pass from debug log:

BB.0    BB.3 (unreachable)
 |     /    \
BB.1(loops)  BB.2 -> ret

Since BB.1 defines %0 and BB.2 reads it, LiveIntervalAnalysis complains about
the undefined value in the unreachable codepath.

First visibly bad commit:
af82d01 [RISCV] Separate doLocalPostpass into new pass...

I think this is just because LiveIntervalAnalysis isn't run before this commit.

One step further back is the commit that causes bb to be split into bb.3.
2c18570 [RISCV] Remove setJumpIsExpensive()

Can be mitigated by -mllvm -jump-is-expensive=true

@topperc
Copy link
Collaborator

topperc commented May 29, 2024

CC @BeMg @lukel97

@lukel97 lukel97 self-assigned this May 29, 2024
@lukel97
Copy link
Contributor

lukel97 commented May 29, 2024

On >=O1 those dead blocks are removed before LiveIntervalAnalysis. But I think the correct thing to do here is to teach RISCVInsertVSETVLI to work without LiveIntervals at O0. That way we can keep debug builds fast.

lukel97 added a commit to lukel97/llvm-project that referenced this issue May 30, 2024
At O0 LiveIntervals isn't usually computed, but after moving RISCVInsertVSETVLI to after phi elimination we've added it as a dependency.

This removes the dependency on LiveIntervals so O0 builds can stay fast, and also avoids a crash caused by LiveIntervals being run at O0 when I'm not sure if ti was designed to be.

The only parts of RISCVInsertVSETVLI that actually use the LiveIntervals analysis are computeInfoForInstr and getInfoForVSETVLI, which lookup val nos for each register AVL use.

At O0 we can emulate this by conservatively returning fake but unique val nos, which will give us suboptimal but otherwise correct codegen.

We need to make sure that we return the same val no given the same MachineInstr, otherwise computeInfoForInstr/getInforForVSETVLI won't be stable across MachineInstrs and the dataflow analysis will fail to converge.

Fixes llvm#93587
preames added a commit to preames/llvm-project that referenced this issue Jun 6, 2024
Stacked on llvm#94658.

We recently moved RISCVInsertVSETVLI from before vector register allocation to after vector register allocation.  When doing so, we added an unconditional dependency on LiveIntervals - even at O0 where LiveIntevals hadn't previously run.  As reported in llvm#93587, this was apparently not safe to do.

This change makes LiveIntervals optional, and adjusts all the update code to only run wen live intervals is present.  The only real tricky part of this change is the abstract state tracking in the dataflow.  We need to represent a "register w/unknown definition" state - but only when we don't have LiveIntervals.

This adjust the abstract state definition so that the AVLIsReg state can represent either a register + valno, or a register + unknown definition.  With LiveIntervals, wehave an exact definition for each AVL use.  Without LiveIntervals, we  treat the definition of a register AVL as being unknown.

The key semantic change is that we now have a state in the lattice for which something is known about the AVL value, but for which two identical lattice elements do *not* neccessarily represent the same AVL value at runtime.  Previously, the only case which could result in such an unknown AVL was the fully unknown state (where VTYPE is also fully unknown).  This requires a small adjustment to hasSameAVL and lattice state equality to draw this important distinction.

The net effect of this patch is that we remove the LiveIntervals dependency at O0, and O0 code quality will regress for cases involving register AVL values.

This patch is an alternative to llvm#93796 and llvm#94340.  It is very directly inspired by review conversation around them, and thus should be considered coauthored by Luke.
preames added a commit to preames/llvm-project that referenced this issue Jun 17, 2024
We recently moved RISCVInsertVSETVLI from before vector register allocation to after vector register allocation.  When doing so, we added an unconditional dependency on LiveIntervals - even at O0 where LiveIntevals hadn't previously run.  As reported in llvm#93587, this was apparently not safe to do.

This change makes LiveIntervals optional, and adjusts all the update code to only run wen live intervals is present.  The only real tricky part of this change is the abstract state tracking in the dataflow.  We need to represent a "register w/unknown definition" state - but only when we don't have LiveIntervals.

This adjust the abstract state definition so that the AVLIsReg state can represent either a register + valno, or a register + unknown definition.  With LiveIntervals, wehave an exact definition for each AVL use.  Without LiveIntervals, we  treat the definition of a register AVL as being unknown.

The key semantic change is that we now have a state in the lattice for which something is known about the AVL value, but for which two identical lattice elements do *not* neccessarily represent the same AVL value at runtime.  Previously, the only case which could result in such an unknown AVL was the fully unknown state (where VTYPE is also fully unknown).  This requires a small adjustment to hasSameAVL and lattice state equality to draw this important distinction.

The net effect of this patch is that we remove the LiveIntervals dependency at O0, and O0 code quality will regress for cases involving register AVL values.  In practice, this means we pessimize code written with intrinsics at O0.

This patch is an alternative to llvm#93796 and llvm#94340.  It is very directly inspired by review conversation around them, and thus should be considered coauthored by Luke.
preames added a commit that referenced this issue Jun 17, 2024
Stacked on #94658.
    
We recently moved RISCVInsertVSETVLI from before vector register
allocation to after vector register allocation. When doing so, we added
an unconditional dependency on LiveIntervals - even at O0 where
LiveIntevals hadn't previously run. As reported in #93587, this was
apparently not safe to do.
    
This change makes LiveIntervals optional, and adjusts all the update
code to only run wen live intervals is present. The only real tricky
part of this change is the abstract state tracking in the dataflow. We
need to represent a "register w/unknown definition" state - but only
when we don't have LiveIntervals.
    
This adjust the abstract state definition so that the AVLIsReg state can
represent either a register + valno, or a register + unknown definition.
With LiveIntervals, we have an exact definition for each AVL use.
Without LiveIntervals, we treat the definition of a register AVL as
being unknown.
    
The key semantic change is that we now have a state in the lattice for
which something is known about the AVL value, but for which two
identical lattice elements do *not* necessarily represent the same AVL
value at runtime. Previously, the only case which could result in such
an unknown AVL was the fully unknown state (where VTYPE is also fully
unknown). This requires a small adjustment to hasSameAVL and lattice
state equality to draw this important distinction.
    
The net effect of this patch is that we remove the LiveIntervals
dependency at O0, and O0 code quality will regress for cases involving
register AVL values.
    
This patch is an alternative to
#93796 and
#94340. It is very directly
inspired by review conversation around them, and thus should be
considered coauthored by Luke.
preames added a commit that referenced this issue Jun 17, 2024
(Reapplying with corrected commit message)

We recently moved RISCVInsertVSETVLI from before vector register allocation
to after vector register allocation.  When doing so, we added an unconditional
dependency on LiveIntervals - even at O0 where LiveIntevals hadn't previously
run.  As reported in #93587, this was apparently not safe to do.

This change makes LiveIntervals optional, and adjusts all the update code to
only run wen live intervals is present.  The only real tricky part of this
change is the abstract state tracking in the dataflow.  We need to represent
a "register w/unknown definition" state - but only when we don't have
LiveIntervals.

This adjust the abstract state definition so that the AVLIsReg state can
represent either a register + valno, or a register + unknown definition.
With LiveIntervals, we have an exact definition for each AVL use.  Without
LiveIntervals, we  treat the definition of a register AVL as being unknown.

The key semantic change is that we now have a state in the lattice for which
something is known about the AVL value, but for which two identical lattice
elements do *not* neccessarily represent the same AVL value at runtime.
Previously, the only case which could result in such an unknown AVL was the
fully unknown state (where VTYPE is also fully unknown).  This requires a
small adjustment to hasSameAVL and lattice state equality to draw this
important distinction.

The net effect of this patch is that we remove the LiveIntervals dependency
at O0, and O0 code quality will regress for cases involving register AVL values.
In practice, this means we pessimize code written with intrinsics at O0.

This patch is an alternative to #93796 and #94340.  It is very directly
inspired by review conversation around them, and thus should be considered
coauthored by Luke.
@preames
Copy link
Collaborator

preames commented Jul 15, 2024

Should be fixed by previously referenced commit.

@llvmbot
Copy link
Member

llvmbot commented Jul 15, 2024

@llvm/issue-subscribers-backend-risc-v

Author: Patrick O'Neill (patrick-rivos)

Reduced LLVM IR: ``` define i16 @f() { BB: br label %BB1

BB1: ; preds = %BB1, %BB
%A = or i16 0, 0
%B = fcmp true float 0.000000e+00, 0.000000e+00
%C = or i1 %B, false
br i1 %C, label %BB1, label %BB2

BB2: ; preds = %BB1
ret i16 %A
}

Godbolt: https://godbolt.org/z/7GGzqs5c6

BB state before the LiveIntervalAnalysis pass from debug log:

BB.0 BB.3 (unreachable)
| /
BB.1(loops) BB.2 -> ret


Since BB.1 defines %0 and BB.2 reads it, LiveIntervalAnalysis complains about
the undefined value in the unreachable codepath.

First visibly bad commit:
af82d01fbbce7808605f3a8b22dd1ca7fdec7886 [RISCV] Separate doLocalPostpass into new pass...

I think this is just because LiveIntervalAnalysis isn't run before this commit.

One step further back is the commit that causes bb to be split into bb.3.
2c185709bca195f3d5e062819bba5dd828330548 [RISCV] Remove setJumpIsExpensive()

Can be mitigated by `-mllvm -jump-is-expensive=true`
</details>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants