Page MenuHomePhabricator

[GISel] Add fpext/fptrunc combines
Needs ReviewPublic

Authored by pnappa on Jul 6 2021, 8:23 PM.

Details

Summary

Adds the following combines, plus AArch64 tests:

  • (fptrunc (fpext x)) -> x, when dest and source types match.
  • (fptrunc ([us]itofp x)) -> ([us]itofp x)
  • (fpext C) -> C
  • (fpext ([us]itofp x)) -> ([us]itofp x), when x is 8 bit.

Diff Detail

Event Timeline

pnappa created this revision.Jul 6 2021, 8:23 PM
pnappa requested review of this revision.Jul 6 2021, 8:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2021, 8:23 PM
pnappa updated this revision to Diff 356866.Jul 6 2021, 10:17 PM

Fixed AMDGPU's fdiv.f16.ll test.

ninja check-llvm-codegen

Successfully passes locally.

aemerson added inline comments.Jul 6 2021, 11:29 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
355

The apply functions should return void.

llvm/include/llvm/Target/GlobalISel/Combine.td
480

likewise, the apply clause doesn't need to return.

paquette added inline comments.Jul 7 2021, 3:29 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2508

It's unfortunate that there's nothing in MIPatternMatch for G_SITOFP and G_UITOFP right now, because it looks like this would work nicely with mi_match.

2533

May be worth reducing indentation here for the sake of slight readability gains.

Also there are some extra braces here which aren't needed.

paquette added inline comments.Jul 7 2021, 3:32 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2548

The apply functions here are basically identical. To reduce duplication, I think it would be better to either

a) Make a helper which handles this type of combine
b) Use applyBuildFn

paquette added inline comments.Jul 7 2021, 3:35 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1680

I think that we should have an assert that we get one of the expected opcodes here.

pnappa updated this revision to Diff 357126.Jul 7 2021, 8:30 PM

Addressed @aemerson & @paquette's feedback.

pnappa marked 5 inline comments as done.Jul 7 2021, 8:30 PM
foad added a subscriber: foad.Jul 8 2021, 3:35 AM
foad added inline comments.
llvm/include/llvm/Target/GlobalISel/Combine.td
353

I didn't know we had these as combines. Do we really want this, instead of constant-folding them in CSEMIRBuilder? See D99036 and D104528 for some steps in that direction.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2531

Is there a precedent for using "UnsafeFPMath" to enable this particular unsafe combine?

paquette added inline comments.Jul 8 2021, 5:54 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2531

This looks really similar to the isKnownExactCastIntToFP case in InstCombinerImpl::visitFPExt.

Taking a peek in isKnownExactCastIntToFP doesn't have any mention of unsafe math. I couldn't find anything in SelectionDAG either (but maybe I missed something?)

(The checks in isKnownExactCastIntToFP seem to do a lot of work actually; I wonder if it would make sense to do something similar here? I guess that could be a follow-up patch.)

pnappa added inline comments.Jul 8 2021, 8:18 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
353

I don't know the answer to this, any one else want to chime in?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2531

Precedent, as in have we done something similar before, enabling a different combine depending on fastmath? Or as in, does this optimisation help in practice?

Yes to both, I think, but the latter is fairly niche

foad added inline comments.Jul 9 2021, 1:21 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2531

Precedent as in, does the unsafe math flag enable this specific optimization anywhere else in the compiler, e.g. on IR or in SelectionDAG? I'm not sure it's a good idea to add new unsafe optimizations into the existing (pretty well established) unsafe math flag.

llvm/include/llvm/Target/GlobalISel/Combine.td
353

I think we've tried constant folding size changing ops (such as ZEXT) in the builder and it has the potential to cause infinite loops in the legalizer algorithm.
In general, since the builder doesn't care about the legality of ops it's folding and producing, we should be a bit careful on what transformations we introduce as part of building instructions.

pnappa updated this revision to Diff 368260.Mon, Aug 23, 8:04 PM

Addressed @foad 's concerns about fast-math. Removed the combine applying when unsafe math is enabled.

foad added a comment.Tue, Aug 24, 2:41 AM

Removed the combine applying when unsafe math is enabled.

Thanks. Please remove it from the commit message too.

llvm/include/llvm/Target/GlobalISel/Combine.td
483

Is this safe? E.g. does converting i32->f32->f16 always give the same result as i32->f16, even though the former does a kind of double rounding (from 32 bits to 24 bit mantissa to 11 bit mantissa)? Does SelectionDAG do the equivalent optimization?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2508

Could return early here.

2531

Could return early here.

pnappa edited the summary of this revision. (Show Details)Tue, Aug 24, 3:05 PM
pnappa marked an inline comment as done.Tue, Aug 24, 4:09 PM
pnappa added inline comments.
llvm/include/llvm/Target/GlobalISel/Combine.td
483

Re: correctness: https://alive2.llvm.org/ce/z/7xZjnH, https://alive2.llvm.org/ce/z/azpy9w. Yeah, it's sound.

I don't know if SDAG does the equivalent optimisation, I had a look in DAGCombiner.cpp, and can't see it. I don't have any experience with SDAG, unfortunately.

pnappa updated this revision to Diff 368493.Tue, Aug 24, 4:14 PM
pnappa marked an inline comment as done.
pnappa marked 2 inline comments as done.
pnappa updated this revision to Diff 368552.Tue, Aug 24, 9:48 PM

Rebased to main. Sorry for the noise!

foad added inline comments.Wed, Aug 25, 2:33 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
483

Re: correctness: https://alive2.llvm.org/ce/z/7xZjnH, https://alive2.llvm.org/ce/z/azpy9w. Yeah, it's sound.

That's not a good enough test. You're only testing an i16 input, which is small enough to fit exactly in the 24-bit float mantissa without being rounded off.

foad added inline comments.Wed, Aug 25, 7:20 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
483

Here's a counterexample: https://godbolt.org/z/h14bob8jd

foad added inline comments.Wed, Aug 25, 7:27 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
483

And here's alive2 saying the same thing: https://alive2.llvm.org/ce/z/UEZDuD

So, I think this might be safe for the specific case of float->half (*) but I don't think it's safe in general for any two floating point types.

(*) Because all integers up to the largest value you can represent as a half (about 2^16) are exactly representable as a float.