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

Prevent InvalidBitWidth Errors #963

Merged
merged 17 commits into from
Feb 6, 2025
Merged

Prevent InvalidBitWidth Errors #963

merged 17 commits into from
Feb 6, 2025

Conversation

pvelesko
Copy link
Collaborator

During optimization of loops, LLVM generates non-standard integer types such as i33 or i56.

Tried to find which LLVM pass does this but it seems to happen quite early.

  1. Upgrade non-standard sizes
  2. Remove redundant trunc instructions

@pvelesko pvelesko force-pushed the fixInvalidBitWidth branch 2 times, most recently from 775f39d to 15a1bf9 Compare January 20, 2025 10:23
@pjaaskel pjaaskel requested a review from linehill January 20, 2025 11:22
@pjaaskel
Copy link
Collaborator

@pvelesko pvelesko changed the base branch from openmm-fixes to main January 20, 2025 12:35
llvm_passes/HipPromoteInts.cpp Outdated Show resolved Hide resolved
@pvelesko pvelesko requested a review from linehill January 23, 2025 07:08
llvm_passes/HipPromoteInts.cpp Outdated Show resolved Hide resolved
llvm_passes/HipPromoteInts.cpp Outdated Show resolved Hide resolved
llvm_passes/HipPromoteInts.cpp Outdated Show resolved Hide resolved
llvm_passes/HipPromoteInts.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@linehill linehill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, run git-clang-format on the changes.

@pvelesko pvelesko force-pushed the fixInvalidBitWidth branch 2 times, most recently from e62dca7 to 85f5553 Compare January 30, 2025 08:45
@pvelesko
Copy link
Collaborator Author

@linehill I spent a fair amount of looking already but to confirm: there is no way to instruct LLVM to NOT generate illegal integer types?

depChain.txt Outdated Show resolved Hide resolved
@linehill
Copy link
Collaborator

linehill commented Feb 3, 2025

@linehill I spent a fair amount of looking already but to confirm: there is no way to instruct LLVM to NOT generate illegal integer types?

Did you look into the pass responsible for spawning the non-standard integers? It might have a command line option for it.

@pvelesko
Copy link
Collaborator Author

pvelesko commented Feb 3, 2025

Did you look into the pass responsible for spawning the non-standard integers? It might have a command line option for it.

I did but I can't find my notes on which one it was but iirc it was quite early and deals with general loop optimizations so disabling is not desired.

@linehill
Copy link
Collaborator

linehill commented Feb 4, 2025

I did but I can't find my notes on which one it was but iirc it was quite early and deals with general loop optimizations so disabling is not desired.

When you find it again, check if it has an option that could stop it to generate non-standard integers - not to disable the whole pass.

@pvelesko
Copy link
Collaborator Author

pvelesko commented Feb 4, 2025

I did but I can't find my notes on which one it was but iirc it was quite early and deals with general loop optimizations so disabling is not desired.

When you find it again, check if it has an option that could stop it to generate non-standard integers - not to disable the whole pass.

It's IndVarSimplifyPass - none of the options I tried seem to help

@pvelesko pvelesko requested a review from linehill February 4, 2025 16:04
Comment on lines 365 to 368
for (auto It = Replacements.rbegin(); It != Replacements.rend();
++It) {
It->Old->eraseFromParent();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to iterate the container in reverse order?

Suggested change
for (auto It = Replacements.rbegin(); It != Replacements.rend();
++It) {
It->Old->eraseFromParent();
}
for (auto &R : Replacements)
R.Old->eraseFromParent();

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To preserve iterators traversing instructions from leaf to root

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why? I'm thinking you encountered restriction on the Instruction::eraseFromParent() that does not allow instructions with users to be erased and the reverse iteration order works-around it. I guess that explains it.

@pvelesko pvelesko requested a review from linehill February 5, 2025 12:37
Copy link
Collaborator

@linehill linehill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@pvelesko pvelesko merged commit e95ea1f into main Feb 6, 2025
36 checks passed
@pvelesko pvelesko deleted the fixInvalidBitWidth branch February 6, 2025 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants