Differential D61802
[LoopVectorize] Enable float minmax reductions via instruction flag Authored by nlw0 on May 10 2019, 1:04 PM.
Details Reduction loops with the min function get vectorized today for integers and floats, but in the second case the function attribute "no-nans-fp-math" is required. This patch makes the vectorization for floats not strictly dependent on that, further enabling it if fcmp has the nnan instruction flag. This issue is alluded to in bug report #35538 https://bugs.llvm.org/show_bug.cgi?id=35538#c1 .
Diff Detail Event TimelineComment Actions [adding potential reviewers who are likely more familiar with the vectorizers than me]
Comment Actions I had not run make check, sorry I'm still a bit lost. There is a number of broken tests. Is the idea that these might be all be benign changes due to the vectorization not kicking in before, and we might just need to update what the generated code now looks like? And then the test file you already included would now be just another test in such state? How do I update this PR now, just apply the patch over current master and upload a new patch file? Comment Actions The problem is that the patch does not check that 'I' is an FPMathOperator before asking if 'I->hasNoNaNs()', so we are crashing (asserting) on a bunch of existing tests. The logic should look like this to work with the current state of FMF: diff --git a/llvm/lib/Analysis/IVDescriptors.cpp b/llvm/lib/Analysis/IVDescriptors.cpp index 19f1a771b85..b6de4ace07c 100644 --- a/llvm/lib/Analysis/IVDescriptors.cpp +++ b/llvm/lib/Analysis/IVDescriptors.cpp @@ -586,8 +586,9 @@ RecurrenceDescriptor::isRecurrenceInstr(Instruction *I, RecurrenceKind Kind, LLVM_FALLTHROUGH; case Instruction::FCmp: case Instruction::ICmp: - if (Kind != RK_IntegerMinMax && - (!HasFunNoNaNAttr || Kind != RK_FloatMinMax)) + bool MightHaveNans = !HasFunNoNaNAttr && + I->getOpcode() == Instruction::FCmp && !I->hasNoNaNs(); + if (Kind != RK_IntegerMinMax && (Kind != RK_FloatMinMax || MightHaveNans)) return InstDesc(false, I); return isMinMaxSelectCmpPattern(I, Prev); } Unfortunately, this patch is exposing existing problems with our instruction-level fast-math-flags. See: So the vectorization code already has the potential to miscompile because we can't just check for no-NANs (nnan). We also need to check for no-signed-zeros (nsz). But checking for nsz should happen on the 'select' rather than the 'fcmp'. I think we should fix the fundamental problems in the IR itself instead of trying to hack around them any more. Comment Actions My main motivation is just enabling vectorization for "minimum" loops without requiring function attributes. I think having a focus might help ensure I can contribute something, and a small patch would be nice too. I'll gladly work on a larger issue if it's within my reach, though, or if turns out to be required. Where can I find more information about the expected behavior of fcmp? I don't understand why signed zeros should be relevant, the resulting behavior should just be whatever is caused by fcmp, either ignoring them or not. And ignoring should be the default anyway from what I recall from IEEE-754. Anyway, isn't this test here just about the fact NaNs violate associativity? Why would select take a flag, isn't the whole semantics encoded in the boolean returned from fcmp? Comment Actions No problem - I've been putting the underlying changes off for a while, so I'll try to make some progress now and cc you on those patches.
Yes, you're correct that fcmp should ignore signed zeros based on IEEE-754, and that's part of the problem: currently, we use 'nsz' on an fcmp to predicate at least 1 transform (that's PR38086).
I think it would be better to discuss more general questions in PR38086 or 1 of the related bugs or on llvm-dev. Comment Actions This is excellent, thanks a lot. There's no hurry. I'll try to familiarize myself with PR38086. Comment Actions
This is solved. Comment Actions It's true that we now allow FMF on select and phi, but this example shows that we're still a long way from deprecating FMF on fcmp. The IR in these tests matches something like this in C source: float f(float *p) {
float min = p[0];
for (unsigned i=0; i<65537; ++i)
if (p[i] < min) min = p[i];
return min;
}When that begins life as IR in clang, there are no select or phi instructions, so existing FMF propagations are never even exercised. We will need to allow FMF on loads and arguments (effectively any FP value) to make this work without using any function attributes. Comment Actions Hi, I'm unable to spend much time on this right now, although I'm definitely still interested. On Julia the IR is currently generated with flags in the fcmp instruction. I was hoping at first that this would just work like that, although now it sounds like there will need to be changes on how this IR is being produced in the first place. What is the way forward? Comment Actions The current thinking is that we want any FP value to be able to carry fast-math-flag properties. That would hopefully remove logical inconsistencies such as shown here: We've made some progress towards that goal by allowing FMF on select and phi instructions. Originally, I thought that just adding FMF to select/phi would be enough to fix this. But it seems clear now that we're going to need to allow FMF on memory ops and probably on function arguments before we can deprecate FMF on fcmp. |