Page MenuHomePhabricator

[SelectionDAG] Teach SelectionDAG::FoldConstantArithmetic to handle SPLAT_VECTOR

Authored by craig.topper on Wed, Mar 31, 1:43 PM.



This allows FoldConstantArithmetic to handle SPLAT_VECTOR in
addition to BUILD_VECTOR. This allows it to support scalable
vectors. I'm also allowing fixed length SPLAT_VECTOR which is
used by some targets, but I'm not familiar enough to write tests
for those targets.

I had to block this function from running on CONCAT_VECTORS to
avoid calling getNode for a CONCAT_VECTORS of 2 scalars.
This can happen because the 2 operand getNode calls this
function for any opcode. Previously we were protected because
before that call. But it's not always possible to fold a CONCAT_VECTORS
of SPLAT_VECTORs, and we don't even try.

This fixes PR49781 where DAG combine thought constant folding
should be possible, but FoldConstantArithmetic couldn't do it.

Diff Detail

Event Timeline

craig.topper created this revision.Wed, Mar 31, 1:43 PM
craig.topper requested review of this revision.Wed, Mar 31, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 31, 1:43 PM
nagisa added a subscriber: nagisa.Wed, Mar 31, 3:36 PM

Please rebase on top of master to verify this fixes the issue mentioned in D88785.

All looks reasonable to me. I'm not sure whether we should do one iteration for fixed-length SPLAT_VECTORs: it should be sound. I don't know what it would gain us given how infrequently it's used.

I'm also not sure whether there's another way of handling the CONCAT_VECTORs issue so hopefully someone else would be able to provide some insight there.

david-arm added inline comments.Thu, Apr 1, 1:46 AM

I wonder if it makes more sense to only set NumOps to VT.getVectorNumElements() if at least one opcode is BUILD_VECTOR. Then you can have an assert that for scalable vectors neither of the opcodes is BUILD_VECTOR.


This is just a suggestion, but instead of checking for scalable vectors, if you change NumOps above to be always be 1 if no BUILD_VECTORs are involved, then maybe you could write:

if (Outputs.size() == 1) {
  // Must be a combination of either SPLAT_VECTOR or UNDEF.
  return getSplatVector(VT, SDLoc(), Outputs[0]);

However, I'm not sure about the case when both inputs are UNDEF and maybe still want to return UNDEF?

craig.topper added inline comments.Thu, Apr 1, 8:38 AM

Both inputs being undef doesn't go through this code, it's handled earlier. One of the operands must be a build_vector or splat_vector.

I think using Outputs.size() == 1 would be incorrect if the fixed vector type has 1 element and the input is build_vector. We shouldn't generate splat_vector in that case.

david-arm added inline comments.Thu, Apr 1, 8:44 AM

Ah ok, fair enough. I was just thinking that it might be nice to avoid tying scalable vectors to SPLAT_VECTOR that's all, given that your changes above should work for fixed-length vectors too. For example, if we've already gone to the trouble of creating inputs for fixed-length vectors, then it might make sense to generate one here too?


Will work on generating splat vectors for fixed vectors when possible next.

Create SPLAT_VECTOR result for SPLAT_VECTOR inputs.

LGTM! Thanks a lot for making the changes.


nit: Perhaps this should be something like:

If one of the inputs is a fixed length SPLAT_VECTOR ...

since at least one input is BUILD_VECTOR?

craig.topper added inline comments.Tue, Apr 6, 10:00 AM

I think I just forgot to delete that TODO when I refactored.

Remove stale TODO

@david-arm, you wrote LGTM, but didn't accept the revision. Do I need someone else to approve?

david-arm accepted this revision.Wed, Apr 7, 12:47 AM

Ah, sorry, that was just a simple mistake and forget to "Accept Revision"!

This revision is now accepted and ready to land.Wed, Apr 7, 12:47 AM