Page MenuHomePhabricator

[InstCombine] canonicalize bitcast after insertelement into undef
ClosedPublic

Authored by spatel on Apr 30 2020, 6:25 AM.

Details

Summary

We have a partial transform in the opposite direction, so that needs to be removed while adding a more general transform that moves bitcast after insertelement.

The motivating case from PR45748:
https://bugs.llvm.org/show_bug.cgi?id=45748
...is the last test diff. In that example, we are triggering an existing bitcast transform, so we reduce the number of casts, and that should give us the ideal x86 codegen.

I'm not sure what to do about the mmx diffs. If the x86 backend is expecting something in particular, we need to specify that here (do we need to exclude/add the mmx type to either of these code diffs?).

Diff Detail

Event Timeline

spatel created this revision.Apr 30 2020, 6:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2020, 6:25 AM

I messed with mmx in this area once and regretted it. See rG5ebbabc1af360756f402203ba7704bb480f279a7

Is canonicalizing towards a vector type that wasn't mentioned in the IR the right way to go? Is that cast free for legal types on all targets? I think its potentially scalarized or becomes a load/store to stack temporary for illegal types in the backend.

I messed with mmx in this area once and regretted it. See rG5ebbabc1af360756f402203ba7704bb480f279a7

Is canonicalizing towards a vector type that wasn't mentioned in the IR the right way to go? Is that cast free for legal types on all targets? I think its potentially scalarized or becomes a load/store to stack temporary for illegal types in the backend.

It kinda sounds to me like such cases should use some custom intrinsics instead of generic bitcasts.

I messed with mmx in this area once and regretted it. See rG5ebbabc1af360756f402203ba7704bb480f279a7

Sufficiently scared. Will remove mmx diffs.

Is canonicalizing towards a vector type that wasn't mentioned in the IR the right way to go? Is that cast free for legal types on all targets? I think its potentially scalarized or becomes a load/store to stack temporary for illegal types in the backend.

I think we're fine here. The vector bitcast that we are creating is only changing the element type (FP <-> int), and we assume that both of those are ok since they are already in the code independent of this transform.
The vector cast is likely safer than the scalar cast on most targets because it's probably not causing any underlying register move (assuming vector registers hold both int and FP). So even if the vector type is illegal, the obvious lowering is a no-op cast to the legal vector type.

spatel updated this revision to Diff 261290.Apr 30 2020, 10:58 AM

Patch updated:
Kept existing fold for mmx and added warning comment.

lebedev.ri accepted this revision.Wed, May 6, 12:07 AM

LG in general, but MMX stuff puzzles me, so would be good for @craig.topper to comment.

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2487–2488 ↗(On Diff #261290)

Am i reading this correctly that this is NFC in context of rG5ebbabc1af360756f402203ba7704bb480f279a7?
Can this be split off?

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
1064

I guess this won't work for scalable vectors?
Can we somehow just replace the elt type in VecOp->getType() instead?

This revision is now accepted and ready to land.Wed, May 6, 12:07 AM
spatel marked 4 inline comments as done.Wed, May 6, 7:50 AM
spatel added a subscriber: ctetreau.
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2487–2488 ↗(On Diff #261290)

It's not quite NFC - this code diff results in the test diff in llvm/test/Transforms/InstCombine/bitcast-vec-canon.ll test "@c". That does seem like an independent improvement though, so I can break that into a preliminary commit.

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
1064

I think this is safe for scalable vectors, so I better add a test. See similar construct annotated below - around line 1279.
cc @ctetreau

1315

Safe for scalable vectors?

spatel updated this revision to Diff 262415.Wed, May 6, 10:31 AM
spatel marked an inline comment as done.

Patch updated:

  1. Pulled the MMX diff into a preliminary commit - rG2058c98715f6
  2. Added a scalable vector test.
This revision was automatically updated to reflect the committed changes.