AMDGPU should implement IR version of isFMAFasterThanFMulAndFAdd

[arsenm]

TargetLowering has an IR variant of isFMAFasterThanFMulAndFAdd, and AMDGPU does not yet implement it.

As far as I can tell, this is only ever used in target code to implement isProfitableToHoist (e.g. AArch64 here https://github.com/llvm/llvm-project/blob/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L16086)

So for this to be effective, isProfitableToHoist would also need to be implemented similarly.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Matt Arsenault (arsenm)

TargetLowering has an IR variant of [isFMAFasterThanFMulAndFAdd](https://github.com/llvm/llvm-project/blob/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/llvm/include/llvm/CodeGen/TargetLowering.h#L3262), and AMDGPU does not yet implement it.

As far as I can tell, this is only ever used in target code to implement isProfitableToHoist (e.g. AArch64 here https://github.com/llvm/llvm-project/blob/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L16086)

So for this to be effective, isProfitableToHoist would also need to be implemented similarly.

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

TargetLowering has an IR variant of [isFMAFasterThanFMulAndFAdd](https://github.com/llvm/llvm-project/blob/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/llvm/include/llvm/CodeGen/TargetLowering.h#L3262), and AMDGPU does not yet implement it.

As far as I can tell, this is only ever used in target code to implement isProfitableToHoist (e.g. AArch64 here https://github.com/llvm/llvm-project/blob/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L16086)

So for this to be effective, isProfitableToHoist would also need to be implemented similarly.

[chinmaydd]

Hi @arsenm, I'm looking at the TargetLowering variant of isFMAFasterThanFMulAndFAdd. It seems to me that most of the logic involves looking at the subtarget properties (such as the denormal mode) which are unavailable at the IR level.

Also, the default behavior of isProfitableToHoist is true -- so we would be adding handling for cases where we dont want to hoist instructions (such as multiply in this case) out of a then/else into the if block.

I would appreciate it if you could give an example where adding an IR version of isFMAFasterThanFMulAndFadd is generally useful.

Thanks !

[arsenm]

Hi @arsenm, I'm looking at the TargetLowering variant of isFMAFasterThanFMulAndFAdd.

It seems to me that most of the logic involves looking at the subtarget properties (such as the denormal mode)
The denormal mode is not a property of the subtarget

which are unavailable at the IR level.

This is untrue. Both the denormal-fp-mode and the subtarget are available, especially in SITargetLowering. See Function::getDenormalMode.

Also, the default behavior of isProfitableToHoist is true -- so we would be adding handling for cases where we dont want to hoist instructions (such as multiply in this case) out of a then/else into the if block.

That's a surprising (and possibly bad) default.

I would appreciate it if you could give an example where adding an IR version of isFMAFasterThanFMulAndFadd is generally useful.

Investigating this is the time consuming part of the issue

[chinmaydd]

Thanks for the response.

This is untrue. Both the denormal-fp-mode and the subtarget are available, especially in SITargetLowering. See Function::getDenormalMode.

Ah, thanks for that !

Investigating this is the time consuming part of the issue

Right. I'll continue to investigate.

[Shoreshen]

Hi, I had a look and summarized little bit as follow:

1. Usage:
   • The only place I found uses of isProfitableToHoist is the simplifycfg pass, which is an optimization pass on LLVM-IR layer
   • It is used to judge if same instructions in successor blocks can be hoisted into the current block, if the end of the block is conditional branch or switch
   • By hoist instructions out, it may reduce the chance in instruction selection of being selected together with other instructions (merged with other instructions)
2. In AAarch, the only situation that it consider is if it is FMul used by FAdd/FSub, which can be selected as FMad together. However, AMD backend may have other situations that makes it profitable of remaining the instruction not hoisted.
3. The conditions that AAarch will return false of isProfitableToHoist is all of the following should be satisfied:
   • Instruction is FMul
   • Instruction is used only once
   • The only user is FAdd or FSub
   • FMA is profitable
   • Target arch support instruction opcode of ISD::FMA with the value type of operand 0 of the user
   • either Options.AllowFPOpFusion == FPOpFusion::Fast (enable all FP fusion as long as its profitable) or Options.UnsafeFPMath == true (Allow to produce results that are "less precise" than IEEE allows)

---

Based on the above, what I may concern:

1. Shall we switch the instruction opcode, if there is a possibility of other instruction we do not want it to be hoisted, based on AMD backend (maybe continuously add more cases).
2. Should we stop hoisting if Options.AllowFPOpFusion == FPOpFusion::Fast or Options.UnsafeFPMath == true? Will FMad depreciate the precision?
3. Currently, it is only used in branch hoisting, which do not decrease the number of instructions (only one successor block will be executed). Would there be possible if it will be used in loop hoisting??

[Shoreshen]

@chinmaydd @braw-lee Hi just wanna ask if you still working on this, thanks~

[chinmaydd]

@Shoreshen yes, I am still working on this. I have the code ready, but working on a test that actually benefits from it.

[Shoreshen]

@Shoreshen yes, I am still working on this. I have the code ready, but working on a test that actually benefits from it.

Hi I may provide a case as follow:

; opt -S -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 --fp-contract=fast -hoist-common-insts=true -mtriple=amdgcn -mcpu=gfx1030 %s
define double @_branch(ptr dereferenceable(8) %x, ptr dereferenceable(8) %y, ptr dereferenceable(8) %a) #0 {
entry:
  %0 = load double, ptr %y, align 8
  %cmp = fcmp oeq double %0, 0.000000e+00
  %1 = load double, ptr %x, align 8
  br i1 %cmp, label %if.then, label %if.else


if.then:                                          ; preds = %entry
  %2 = load double, ptr %a, align 8
  %mul = fmul fast double %1, %2
  %add = fadd fast double 1.000000e+00, %mul
  ret double %mul

if.else:                                          ; preds = %entry
  %4 = load double, ptr %a, align 8
  %mul1 = fmul fast double %1, %4
  %sub = fsub fast double %mul1, %0
  ret double %sub
}

The load should be hoisted and fmul should not be~~ Thanks~~