Instead of add (sub) and mul instructions, use v_mad, v_mac or v_fma
if fma instructions are faster and are legal for the given architecture.
Combiner for a simple case that has only one add and one mul instruction
and transforms them into some of the fma instructions depending on the
architecture.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
---|---|---|
3145–3146 | Using bf16 here instead of regular half is wrong. We also have a utility function to get the FP type from the LLT | |
3159 | Checking the denormal mode here isn't right. The denormal mode only impacts whether this should be used for AMDGPU and isn't a generic property | |
3185–3187 | return compare directly | |
3196 | The matching logic here implies this should be able to use fma also |
Instead of add and mul instructions, use v_mad, v_mac or v_fma
if fma instructions are faster and are legal for the given architecture.
Combiner for a simple case that has only one add and one mul instruction
and transforms them into some of the fma instructions depending on the
architecture.
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h | ||
---|---|---|
112 | isLegal needs its own comment. | |
333 | Comment should mention fmad. | |
334 | Capitalise them like "FAdd" and "FMul" and "FMA" and "FMad". There are akready examples of this in GlobalISel. | |
550 | "FMul" | |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
798 | Do you need EVT() here? I thought MVT would implicitly convert to EVT. |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
795 | Function needs a comment. | |
1114 | Function needs a comment. | |
2693 | Function needs a comment. | |
2734 | Function needs a comment. | |
2739 | Function needs a comment. | |
llvm/include/llvm/Target/GlobalISel/Combine.td | ||
560 | "combine_fadd_fmul_to_fmad_or_fma"? | |
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
3575 | Function needs a comment. |
llvm/include/llvm/Target/GlobalISel/Combine.td | ||
---|---|---|
608 | You've lost load_or_combine. |
Put back the accidentally deleted combiner from the list of combiners (load_or_combine).
Added two mir tests (prelegalize and postlegalize combiner) and a few adjustments in CombinerHelper.cpp.
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h | ||
---|---|---|
110–111 | Comment seems wrong. Should it be "... running after legalization ..."? | |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
2717 | "const LLT" seems weird for a parameter type. Is there a reason for the const? | |
2729 | "const LLT" seems weird. But why do we need this isFMADLegal function, which calls into selectiondag legalization code? Can't you use CombinerHelper isLegal methods instead? | |
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
3906 | This could be static, just like isContractable? | |
3910 | return AllowFusionGlobally || isContractable(MI);? | |
3927 | MI.getMF() | |
3939 | This expression is exactly what "isLegalOrBeforeLegalization" does, so you didn't need to create isLegal. | |
3951 | !isContractable already implies !MI.FmReassoc so you don't need to check it again here. | |
llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
3330 ↗ | (On Diff #337401) | Can you submit this change as a separate patch? (Does it affect any existing lit tests?) |
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h | ||
---|---|---|
110–111 | That seems ok, I'll change that. | |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
2717 | I don't think there is a good reason for that, I'll change that. | |
2729 | I'll change the const LLT. | |
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
3906 | It probably should, I'll change that. | |
3910 | Thanks! | |
3927 | Thanks! | |
3939 | "isLegalOrBeforeLegalization" returns the value of "!LI || LI->getAction(Query).Action == LegalizeActions::Legal" | |
3951 | Thanks! | |
llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
3330 ↗ | (On Diff #337401) | Yes, it should be in a separate patch, it was added here by mistake. |
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
---|---|---|
3939 | I mean !LegalOperations || isLegal(...) is the same as isLegalOrBeforeLegalization(...). Because LegalOperations is just another name for LI. |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
2729 | I was hoping that hasFP64FP16Denormals etc would be tested in AMDGPULegalizerInfo::AMDGPULegalizerInfo, where it decides whether G_FMAD is legal, so the normal isLegal* functions would work. But now I see that hasFP64FP16Denormals depends on function attributes, so it can't be done that way. |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
2729 | I'm not sure. Alternatively you could just check "isLegalOrBeforeLegalizer" on the FMAD instructions. Then pre-legalization we would always combine to FMAD, and then the legalizer might decide that the FMAD is not legal and turn it into either MUL+ADD or FMA. Would that work? |
I think this will look good once you've updated the patch for the outstanding minor comments.
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
2729 | We discussed this offline. I still think this would be an interesting experiment to try, but I also think the approach in the current patch is OK for now. |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
2729 | Right, the FP mode is not a property of the target. It's a property of the FP mode of the function, which is contextually valid or not. The legalizer rules are not allowed to differ based on context like that |
Move isFPExtFoldable() to the patch where it is used for the first time.
Updated to reflect changes in D104247.
Update tests.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
---|---|---|
4627–4639 | We could instead use applyBuildFn which uses a lambda as MatchInfo that details what instructions to build. I noticed this is used for some new combines. It might look more readable then this long tuple. Especially for other fma combines. If we do this for other fma combines as well then we could merge multiple matches into one (like the ones that start with fadd D93305, D97937, D97938, D98047) and avoid running a lot of common checks again for each combine (like the ones in canCombineFMadOrFMA). @foad, @arsenm What do you think of this idea. Would it be more preferable? |
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h | ||
---|---|---|
386 | Rename type to BuildFnTy since there is a: (same for other patches as well, might help with formatting) |
LGTM
llvm/include/llvm/Target/GlobalISel/Combine.td | ||
---|---|---|
822–824 | Might be useful to make a new combine group for this and other fma combines. |
Comment seems wrong. Should it be "... running after legalization ..."?