This is an archive of the discontinued LLVM Phabricator instance.

InstCombine: Fold out scale-if-denormal pattern
ClosedPublic

Authored by arsenm on Aug 3 2023, 1:34 PM.

Details

Summary

Fold select (fcmp oeq x, 0), (fmul x, y), x => x

This cleans up a pattern left behind by denormal range checks under
denormals are zero.

The pattern starts out as something like:
x = x < smallest_normal ? x * K : x;

The comparison folds to an == 0 when the denormal mode treats input
denormals as zero. This makes library denormal checks free after
linked into DAZ enabled code.

alive2 is mostly happy with this, but there are some issues. First,
there are many reported failures in some of the negative tests that
happen to trigger some preexisting canonicalize introducing
combine. Second, alive2 is incorrectly asserting that denormals must
be flushed with the DAZ modes. It's allowed to drop a canonicalize.

Diff Detail

Event Timeline

arsenm created this revision.Aug 3 2023, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2023, 1:34 PM
arsenm requested review of this revision.Aug 3 2023, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2023, 1:34 PM
Herald added a subscriber: wdng. · View Herald Transcript
foad added inline comments.Aug 4 2023, 12:49 AM
llvm/lib/Transforms/InstCombine/InstCombineInternal.h
260

Seems a bit strong to say that fmul nsz MulVal, +0.0 will yield +0.0 when MulVal is negative. I think the nsz flag just means that optimizations applied to this instruction don't have to respect the sign of zero. It's still possible that some other later (non-nsz) instruction would observe a -0.0 result from this fmul.

I think the behaviour is fine but the comment could use improving.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
3463

Stray "and"?

goldstein.w.n added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
3499

There is alot of duplicate code between the two if statements.
Maybe fold it into a lambda and pass TrueVal/FalseVal and eq/ne as constraints?
At the very least the actual creation of the new instruction code can be hoisted to some shared codes.

arsenm added inline comments.Aug 4 2023, 8:46 AM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
3316

This is essentially reinventing D151887

arsenm updated this revision to Diff 547357.Aug 4 2023, 2:40 PM
arsenm marked 3 inline comments as done.
goldstein.w.n added inline comments.Aug 16 2023, 12:30 PM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
3324

matchFMulByZeroIfResultEqZero? Seems a lot clearer.

3460

What do you mean by "This drops a canonicalization"? Causes us to miss it?

Can you also describe the "scale-if-equals-zero" pattern. Mostly for my sake, not familiar with it. If another reviewer is they might be better fit for this patch.

3463

Missing something after the "so"?

3479

Imo:

Value * MatchCmp0 = nullptr;
if(Pred == CmpInst::FCMP_OEQ || ...UEQ)
   MatchCmp0 = FalseVal;
else if(Pred == CmpInst::FCMP_ONE || ...UNE)
   MatchCmp0 = TrueVal;
if(Cmp0 == MatchCmp0 && matchFMulByZeroIfEqZero(this, Cmp0, Cmp1, Cmp1, Cmp0, SI))
   return replaceInstUsesWith(SI, Cmp0);
arsenm updated this revision to Diff 551174.Aug 17 2023, 9:27 AM
arsenm marked 3 inline comments as done.
arsenm added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
3460

Before this you always get a value that goes through a multiply, which if we strictly followed FP behavior would require quieting a signaling nan and flushing denormals. By replacing with a direct x reference, that doesn't happen (but per the langref is allowed without strictfp)

The pattern is covered by the select, the multiply if equal 0 pattern

This needs to be rebased.

Code looks good (once rebased) I'll test + accept.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
3326

nit: Would replace SelectInst &SI with Instruction * CxtI here. TrueVal/FalseVal don't actually match the select operands heres which seems bugprone if at somepoint someone thinks "oh I can just get these from the select".

arsenm updated this revision to Diff 554088.Aug 28 2023, 4:11 PM
arsenm marked an inline comment as done.

When I try to check this out locally I'm getting an issue. Is there a missing patch in this series?

$> arc patch D157030
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D157030.
Checking patch llvm/test/Transforms/InstCombine/fold-select-fmul-if-zero.ll...
error: llvm/test/Transforms/InstCombine/fold-select-fmul-if-zero.ll: does not exist in index
Checking patch llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp...
Checking patch llvm/lib/Transforms/InstCombine/InstCombineInternal.h...
Checking patch llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp...
Applied patch llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp cleanly.
Applied patch llvm/lib/Transforms/InstCombine/InstCombineInternal.h cleanly.
Applied patch llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp cleanly.

 Patch Failed! 
Usage Exception: Unable to apply patch!

When I try to check this out locally I'm getting an issue. Is there a missing patch in this series?

$> arc patch D157030
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D157030.
Checking patch llvm/test/Transforms/InstCombine/fold-select-fmul-if-zero.ll...
error: llvm/test/Transforms/InstCombine/fold-select-fmul-if-zero.ll: does not exist in index
Checking patch llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp...
Checking patch llvm/lib/Transforms/InstCombine/InstCombineInternal.h...
Checking patch llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp...
Applied patch llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp cleanly.
Applied patch llvm/lib/Transforms/InstCombine/InstCombineInternal.h cleanly.
Applied patch llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp cleanly.

 Patch Failed! 
Usage Exception: Unable to apply patch!

You don't have the baseline test commit, just pushed it

goldstein.w.n added inline comments.Aug 28 2023, 7:38 PM
llvm/test/Transforms/InstCombine/fold-select-fmul-if-zero.ll
398

Why does this fail?

arsenm updated this revision to Diff 554747.Aug 30 2023, 9:15 AM

Use flag helper now that it's pushed

arsenm marked an inline comment as done.Aug 30 2023, 9:19 AM
arsenm added inline comments.
llvm/test/Transforms/InstCombine/fold-select-fmul-if-zero.ll
398

The nsz needs to be set on the select. nsz on the fmul is insufficient

goldstein.w.n accepted this revision.Aug 30 2023, 12:08 PM

Okay, LGTM.

FP stuff is not an area I'm really qualified to review so it would be nice if a second opinion
also chimed in, but if no one else is up to review think its okay to push.
Please way a few days to push to give other times to review.

llvm/test/Transforms/InstCombine/fold-select-fmul-if-zero.ll
398

Ah yeah that makes sense.

This revision is now accepted and ready to land.Aug 30 2023, 12:08 PM