This is an archive of the discontinued LLVM Phabricator instance.

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
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.

Diff Detail

Event Timeline

arsenm created this revision.Oct 28 2019, 7:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 7:59 PM

Is isFMAFasterThanFMulAndFAdd's MachineFunction argument currently used? Or is that coming in a follow-up Diff? I might have missed it...

llvm/include/llvm/CodeGen/TargetLowering.h
2549

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.

virtual bool isFMAFasterThanFMulAndFAdd(EVT, const MachineFunction &MF = nullptr) const {

Or is that not allowed with some virtual functions? I can't remember the details...

arsenm marked an inline comment as done.Oct 31 2019, 8:22 AM

Is isFMAFasterThanFMulAndFAdd's MachineFunction argument currently used? Or is that coming in a follow-up Diff? I might have missed it...

D69583 starts using it (although it's still effectively a no-op until it ultimately uses a function attribute instead of a subtarget feature)

llvm/include/llvm/CodeGen/TargetLowering.h
2549

That would still be using EVT, which I would prefer to avoid spreading. I also wouldn't like using an optional argument here, because then there's a risk of new uses not passing it

llvm/include/llvm/CodeGen/TargetLowering.h
2549

The intention seems correct, but I don't have a lot of confidence reviewing a change like that. Maybe @bogner does?

cameron.mcinally added a reviewer: eli.friedman.

Adding @spatel and @eli.friedman. My confidence level is low for this review...

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.

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.

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.

spatel accepted this revision.Nov 18 2019, 9:24 AM

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.

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.

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.
It's NFC at this point either way, so LGTM.
Adding a few more potential SDAG reviewers, so wait a day or so to see if anyone else has comments.

This revision is now accepted and ready to land.Nov 18 2019, 9:24 AM