Skip to content
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

LiveVariables improperly handles killing aliased registers #1678

Closed
llvmbot opened this issue Apr 4, 2007 · 11 comments
Closed

LiveVariables improperly handles killing aliased registers #1678

llvmbot opened this issue Apr 4, 2007 · 11 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 4, 2007

Bugzilla Link 1306
Resolution FIXED
Resolved on Mar 06, 2010 13:59
Version unspecified
OS All
Reporter LLVM Bugzilla Contributor
CC @lattner,@sunfishcode

Extended Description

Here is the beginning of the BB dump.

entry (0x8503c80, LLVM BB @​0x8501af0, ID#0):
Live Ins: %R0 %R1
%reg1024 = ORI %R0, 0
%reg1025 = ORI %R1, 0

V4R0 is getting killed because handleLiveInRegister() is called on
all results of getAliasSet() for each of the liveins (this is in
LiveIntervals::computeIntervals() ).

handleRegisterDef() does a similar thing where calls
handlePhysicalRegisterDef() on all members of getAliasSet() returned
for the def, which also triggers this problem.


This is a pretty serious bug. LiveVariables::KillsRegister should not kill aliases that are "larger". The
correct way to fix this is to explicitly list registers that are defined, used, and killed. So your example
should look like:

entry (0x8503c80, LLVM BB @​0x8501af0, ID#0):
Live Ins: %R0 %R1
%reg1024 = ORI %R0, 0, %V4R0
%reg1025 = ORI %R1, 0, %V4R0<imp-use,kill>

KillsRegister should check for exact match rather than regsOverlap. There are probably other similar
bugs in LiveVariables.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Apr 19, 2007

This is causing me much pain.

The correct fix, IMO, is to explicit add every single alias register as use /
def operands. Change LiveVariables and LiveIntervalAnalysis to not iterate
through alias sets when processing registers. Also change
LiveVariables::KillsRegister(), RegisterDefIsDead(), etc. to check for exact
register match rather than regsOverlap().

It's unclear though how copy coalescer handles this though. Anyway, it's a
fairly involved set of changes. Anyone has a better suggestion?

@llvmbot
Copy link
Collaborator Author

llvmbot commented Apr 19, 2007

My brute force fix works well on X86. All of the dejagnu tests are passing as
well. I'll do some more testing on other targets.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Apr 20, 2007

Patch 1

@llvmbot
Copy link
Collaborator Author

llvmbot commented Apr 20, 2007

This is the patch. Chris, please see if it fixes the problems you run into. I'm
working on a better fix.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Apr 23, 2007

The patch cleared up the issue I was seeing.

@lattner
Copy link
Collaborator

lattner commented Apr 23, 2007

Great! Evan is working on a version that will be better for compile time and handle more cases. It should
land in the next week or so.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Apr 26, 2007

This is working for me.

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 1, 2007

Should this be marked closed as it's working for me, or is it waiting for more testing?

@lattner
Copy link
Collaborator

lattner commented May 1, 2007

Yep, evan should close it. He's unruly though, and doesn't like to close bugs ;-)

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 2, 2007

I like to leave bugs open to give the impression that I am busy.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

2 participants