Page MenuHomePhabricator

[DAGCombiner][TargetLowering] FCOPYSIGN mixed types legality
AbandonedPublic

Authored by luismarques on Nov 10 2019, 2:41 PM.

Details

Summary

This patch is an alternative approach than 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. 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 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...

Diff Detail

Event Timeline

luismarques created this revision.Nov 10 2019, 2:41 PM

This isn't quite what I was trying to get at. Whether we emit a table, or use some other underlying mechanism to provide defaults, isn't really important. The key points here are:

  1. There should be a clear query for whether the operation is legal.
  2. We should allow generating unsupported operations before LegalizeDAG runs.
  3. LegalizeDAG should support transforming illegal operations to legal ones.
  1. There should be a clear query for whether the operation is legal.
  2. We should allow generating unsupported operations before LegalizeDAG runs.
  3. LegalizeDAG should support transforming illegal operations to legal ones.

Thanks for the clarification, that makes sense. I will rework the patch.

dmz added a subscriber: dmz.Nov 12 2019, 10:02 AM
lenary resigned from this revision.Jan 14 2021, 11:07 AM
luismarques abandoned this revision.Feb 4 2021, 8:15 AM