This is an archive of the discontinued LLVM Phabricator instance.

[GISel] Add combines for unary FP instrs with constant operand
ClosedPublic

Authored by mkitzan on Aug 21 2020, 11:27 PM.

Details

Summary

Patch adds five new GICombinerRules, one for each of the following unary FP instrs: G_FNEG, G_FABS, G_FPTRUNC, G_FSQRT, and G_FLOG2. The combine rules perform the FP operation on the constant operand and replace the original instr with the result. Patch additionally adds new combiner tests for the AArch64 target to test these new combiner rules.

Diff Detail

Event Timeline

mkitzan created this revision.Aug 21 2020, 11:27 PM
mkitzan requested review of this revision.Aug 21 2020, 11:27 PM
mkitzan updated this revision to Diff 287195.EditedAug 22 2020, 9:20 AM

Fixed clang-tidy feedback
(not enough)

arsenm added inline comments.Aug 22 2020, 10:36 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1481

Why go through double instead of preserving the APFloat?

arsenm added inline comments.Aug 22 2020, 10:44 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1462

Wrong rounding mode

mkitzan added inline comments.Aug 22 2020, 10:46 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1481

Because GIDefMatchData wants to have the variable uninitialized, which would call the default ctor of APFloat which is private.

See following pseudo code:

APFLoat MatchDataN; // calls APFloat()
if (matchCombineConstantFoldFpUnary(MI, MatchDataN))
  replaceInstWithAPFloat(MI, MatchDataN); // dummy function for example

The error looks like:

llvm-project/build/lib/Target/AArch64/AArch64GenPreLegalizeGICombiner.inc:343:11: error: calling a private constructor of class 'llvm::APFloat'
  APFloat MatchData23;
          ^
llvm-project/llvm/include/llvm/ADT/APFloat.h:842:3: note: implicitly declared private here
  APFloat() : U(IEEEdouble()) {
  ^
arsenm added inline comments.Aug 22 2020, 10:57 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1481

I guess you could work around this by keeping it wrapped in Optional<APFloat>

mkitzan added inline comments.Aug 22 2020, 11:27 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1462

Should the correct rounding mode be rmNearestTiesToEven? Is that only for G_FPTRUNC to LLT::scalar(16) and not LLT::scalar(32)?

1481

That could work. I ended up liking the current solution with replaceInstWithFConstant over my initial prototype where I tried passing around the APFloat&, because buildFConstant(DstOp, double) will convert the double to the appropriate APFloat depending on the LLT of the DstOp. That way we can just take advantage of the existing replaceInstWithFConstant function.

mkitzan updated this revision to Diff 287420.Aug 24 2020, 9:27 AM

Rebased and resolved conflicts with new changes to Combine.td.

arsenm added inline comments.Aug 24 2020, 10:26 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1462

All of the non-constrained FP instructions assume rmNearestTiesToEven independent of the type

1481

This adds limitations on supporting other FP types, like fp128. It's best to keep everything in APFloat

mkitzan updated this revision to Diff 287697.Aug 25 2020, 9:52 AM
  • match / apply functions use Optional<APFloat> to pass info about the constant FP.
  • Updated rounding modes to be rmNearestTiesToEven
  • Refactored constantFoldFpUnary
arsenm added inline comments.Aug 26 2020, 4:51 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1563–1566

We should probably have a getFltSemanticForLLT utility somewhere for this

1583

setInstrAndDebugLoc?

mkitzan added inline comments.Aug 27 2020, 7:02 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1563–1566

Doesn't appear to be one for LLT. I suspect because unlike Type, with its discrete number of TypeIDs, LLT is very open ended.

1583

Right. Will pick this up in the next fixup.

mkitzan updated this revision to Diff 288505.Aug 27 2020, 7:28 PM
  • Rebased and fixed merge conflicts
  • Changed setInstr to setInstrAndDebugLoc
arsenm added inline comments.Aug 28 2020, 11:16 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1563–1566

Yes, that's why there should be one. An FP LLT isn't any arbitrary number of bits, there's still only the handful of valid FP type combinations. We're probably going to have to add something to track f16 vs. bf16, but for now it's still a switch over a handful of valid FP sizes

mkitzan updated this revision to Diff 290001.Sep 4 2020, 11:47 AM
  • Added LLT helper function to get float semantic from scalar LLT (IEEE fp semantics only at the moment)
  • Updated constantFoldFpUnary to use the new helper function
  • Rebased / fixed merge conflicts
paquette added inline comments.Sep 4 2020, 2:08 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
294

Is there any reason these are all separate combines, when they're all using the same function? Have you found it useful to be able to turn these on/off per-opcode?

Most other combines look like this:

def fconstant_matchinfo: GIDefMatchData<"Optional<APFloat>">;
def constant_fold_unary: GICombineRule <
  (defs root:$root, fconstant_matchinfo:$info),
  (match (wip_match_opcode G_FNEG, G_FABS, G_FPTRUNC, G_FSQRT, G_FLOG2):$root,
    [{ return Helper.matchCombineConstantFoldFpUnary(*${root}, ${info}); }]),
  (apply [{ return Helper.applyCombineConstantFoldFpUnary(*${root}, ${info}); }])
>;
295

Do you need separate matchinfo definitions?

I've noticed all the combines do this, but I think it would be better to just say

def fconstant_matchinfo: GIDefMatchData<"Optional<APFloat>">;

and then reuse it in every combine that uses it versus redefining matchinfo for every combine. (The rest of the combines could probably be cleaned up similarly in a later commit if this works)

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

This should probably assert or be a llvm_unreachable, since we never expect to run this with any other opcode.

mkitzan added inline comments.Sep 4 2020, 2:43 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
294

No reason, except I didn't see that we could have a list of opcodes. Will fix that.

295

When they are all refactored into a single combine rule, we'll end up with one matchinfo for free.

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

Makes sense to have an assert. If somehow control flow ended up there, then the developer would likely want the compiler to assert rather than return None.

mkitzan updated this revision to Diff 290052.EditedSep 4 2020, 5:12 PM
  • Collapsed the many combine rules into a single combine rule with a list of matchable opcodes
  • Made the default switch case in constantFoldFpUnary be llvm_unreachable
arsenm added inline comments.Sep 9 2020, 8:04 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
295

Tablegen isn't smart enough to reuse identical matchinfos (although this should really be fixed)

arsenm added inline comments.Sep 14 2020, 4:06 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1443

Don't need to bother with the type check?

1459

Why special case S64 and not use getFltSemanticForLLT?

llvm/lib/CodeGen/LowLevelType.cpp
64 ↗(On Diff #290052)

I would expect this to just do .getScalarSizeInBits

mkitzan added inline comments.Sep 14 2020, 6:37 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1443

Probably not anymore

1459

You're right, no longer necessary

llvm/lib/CodeGen/LowLevelType.cpp
64 ↗(On Diff #290052)

I figured calling it on the vector element type would likely be the typical intended use, and calling on the aggregate vector type would likely be bug (and wouldn't really make much sense).

mkitzan updated this revision to Diff 292052.Sep 15 2020, 4:32 PM
  • Rebased and fixed merge conflicts
  • Removed unnecessary type checks in constantFoldFpUnary
  • Removed G_FPTRUNC special case for S64 in constantFoldFpUnary
arsenm accepted this revision.Sep 16 2020, 7:53 AM
This revision is now accepted and ready to land.Sep 16 2020, 7:53 AM