[PATCH] D70064: [DAGCombiner][TargetLowering] FCOPYSIGN mixed types legality

[Luís Marques via Phabricator via llvm-commits]

luismarques created this revision.
luismarques added reviewers: efriedma, asb, lenary.
Herald added subscribers: llvm-commits, sameer.abuasal, pzheng, s.egerton, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, MaskRay, jrtc27, niosHD, sabuasal, apazos, simoncook, johnrusso, rbar, hiraditya.
Herald added a project: LLVM.

This patch is an alternative approach than D66725 <https://reviews.llvm.org/D66725> to the problem of handling FCOPYSIGN with mixed argument types.

As suggested by @efriedma, following the existing target lowering convention for operations with multiple types (e.g. like is done for `getLoadExtAction`) seemed like a fine choice a priori, compared with the hook proposed in D66725 <https://reviews.llvm.org/D66725>. But it seems like it actually proves to be less straightforward.

The issue is whether we want to fold away casts that ensure that the FCOPYSIGN sign arguments have the same type as the magnitude arguments. In D66725 <https://reviews.llvm.org/D66725> I tried to provide a hook that had safe and sensible defaults: it doesn't assume that the target is able to handle mixed types (and thus you can't fold away the "cast"). On the other hand, if the fcopysign was going to be expanded anyway then the hook reports that mixed types are fine, since the generated code handles that fine and we avoid the unneeded extension/rounding operations.

I initially tried to provide similar semantics in this patch, but that proved misguided for several reasons. The defaults for these types of multi-type operation actions are filled in in `TargetLoweringBase::initActions`. When that runs the derived TargetLowering hasn't yet initialized so information about the legality of types isn't yet properly set. After playing with several approaches to provide something similar I gave up and filled in the default actions with the current semantics: the copysign is legal for all type pairs except when the sign is an f128. But it seems less than ideal to promote some current target limitations to a target-independent operation default.

The array of actions is provided in `CopySignActions[MVT::LAST_VALUETYPE][MVT::LAST_VALUETYPE]`. At first I thought I could make that a lot smaller by trimming the array lengths to the range of scalar floating-poing types but some targets also use this stuff with vector floating-point types (which don't have a numerical ID sequential to the scalar ones). Thinking from first principles, having a table or an algorithm that generates the same values should be equivalent. I understand the desire to be consistent with the existing code, but in this case it seems like we end up with a very sparse table, where in practice we only care to modify a few entries. That favours expressing the logic as code instead of as a pre-computed table IMHO.

Does it make sense to keep both `isOperationLegal(ISD::FCOPYSIGN, Ty)` and also `isCopySignLegal(Ty, Ty)`? Given the semantics of the operation, they should be equivalent. It seems like in the existing multi-type tables there wasn't as much of that kind of overlap? I can see them easily end up being inconsistent, even though they should return the same answer.

Is this even a reasonable mechanism to express what we want? It expresses the action (legal, expand, promote, custom...) but we are only using it for the dag combine, not for anything that affects how the FCOPYSIGN is actually lowered. With the hook alternative we moved from some more abstract "can you deal with this" to asking (at the point of the combine) the more concrete "is this combination of types legal"? But we actually only care if we should do the combine, not the exact action. Take the config `setCopySignAction(MVT::f64, MVT::f32, Promote)`. Are we actually promoting the `f32` or are we just not discarding the promotion operation that was there to begin with? As the patch is I could have put another action, as long as it wasn't legal. That sounds fishy. And so on, and so forth...


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70064

Files:
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  llvm/lib/CodeGen/TargetLoweringBase.cpp
  llvm/lib/Target/RISCV/RISCVISelLowering.cpp
  llvm/test/CodeGen/RISCV/copysign-casts.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70064.228623.patch
Type: text/x-patch
Size: 9316 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191110/804bf483/attachment-0001.bin>