AMDGPU needs to know the FP mode for the function to answer this
correctly when this is removed from the subtarget
AArch64 had to make this more complicated by using this from an IR
hook, so add an IR typed overload.
Paths
| Differential D69547
DAG: Add function context to isFMAFasterThanFMulAndFAdd ClosedPublic Authored by arsenm on Oct 28 2019, 7:59 PM.
Details Summary AMDGPU needs to know the FP mode for the function to answer this AArch64 had to make this more complicated by using this from an IR
Diff Detail Event Timelinearsenm added a child revision: D69583: AMDGPU: Refactor treatment of denormal mode.Oct 29 2019, 1:26 PM Comment Actions Is isFMAFasterThanFMulAndFAdd's MachineFunction argument currently used? Or is that coming in a follow-up Diff? I might have missed it...
Comment Actions
D69583 starts using it (although it's still effectively a no-op until it ultimately uses a function attribute instead of a subtarget feature)
arsenm added a child revision: D69878: Consoldiate internal denormal flushing controls.Nov 5 2019, 9:07 PM Comment Actions I might be missing some part of the bigger picture, but shouldn't we nail down the format of the denorm function attribute before this? It seems unnecessarily wide/vague to pass the entire MachineFunction as the param if all we care about is the 1 denorm attribute setting. Comment Actions
The fact that AMDGPU cares about the denormal mode context in this case is a target specific detail. It would be stranger to be passing specifically that in for every user. Another target might have more exotic constraints. AMDGPU does have other specific FP control bits that don't specifically matter here, but another target could have others that do. Comment Actions
Fair enough. I'd choose the other way until there was some proven need to code it this way, but that's just a matter of taste. This revision is now accepted and ready to land.Nov 18 2019, 9:24 AM
Revision Contents
Diff 226833 llvm/include/llvm/CodeGen/TargetLowering.h
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
llvm/lib/Target/AArch64/AArch64ISelLowering.h
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
llvm/lib/Target/AMDGPU/SIISelLowering.h
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
llvm/lib/Target/ARM/ARMISelLowering.h
llvm/lib/Target/Hexagon/HexagonISelLowering.h
llvm/lib/Target/Hexagon/HexagonISelLowering.cpp
llvm/lib/Target/NVPTX/NVPTXISelLowering.h
llvm/lib/Target/PowerPC/PPCISelLowering.h
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
llvm/lib/Target/SystemZ/SystemZISelLowering.h
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
llvm/lib/Target/X86/X86ISelLowering.h
llvm/lib/Target/X86/X86ISelLowering.cpp
|
Agreed, the AArch64 requirements are ugly.
It doesn't look like the AArch64 hooks need the Function arg. Could we just make the MachineFunction argument optional; to consolidate the two functions? E.g.
Or is that not allowed with some virtual functions? I can't remember the details...