-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[LAA] Don't assume libcalls with output/input pointers can be vectorized #108980
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2449,13 +2449,20 @@ bool LoopAccessInfo::analyzeLoop(AAResults *AA, const LoopInfo *LI, | |
continue; | ||
|
||
// If this is a load, save it. If this instruction can read from memory | ||
// but is not a load, then we quit. Notice that we don't handle function | ||
// calls that read or write. | ||
// but is not a load, we only allow it if it's a call to a function with a | ||
// vector mapping and no pointer arguments. | ||
if (I.mayReadFromMemory()) { | ||
// If the function has an explicit vectorized counterpart, we can safely | ||
// assume that it can be vectorized. | ||
auto hasPointerArgs = [](CallBase *CB) { | ||
return any_of(CB->args(), [](Value const *Arg) { | ||
return Arg->getType()->isPointerTy(); | ||
}); | ||
}; | ||
|
||
// If the function has an explicit vectorized counterpart, and does not | ||
// take output/input pointers, we can safely assume that it can be | ||
// vectorized. | ||
if (Call && !Call->isNoBuiltin() && Call->getCalledFunction() && | ||
!VFDatabase::getMappings(*Call).empty()) | ||
!hasPointerArgs(Call) && !VFDatabase::getMappings(*Call).empty()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing that is a bit surprising here is that functions without pointer arguments are marked as mayReadFromMemory. If they don't access memory, shouldn't they be marked as not reading from memory? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps It's also possible that we're back in a position of effectively preventing all functions that access memory but given we have this finer control is there harm in keeping it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a bit, this seems to allow vectorizing some library calls without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (without this patch that includes functions like sincos) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Functions might read from other parts of memory like globals or TU-local data (think lookup tables for some math functions). Or a system clock for a function like There's the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would still be good to know if there is any justification for vectorizing cases with custom veclibs for functions that may access errno but as I said earlier, fixing this can be done as follow up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is intentional, this should definitely be documented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (and then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've posted a short RFC on this here: https://discourse.llvm.org/t/rfc-should-fveclib-imply-fno-math-errno-for-all-targets/81384 |
||
continue; | ||
|
||
auto *Ld = dyn_cast<LoadInst>(&I); | ||
|
Uh oh!
There was an error while loading. Please reload this page.