[PATCH] D112064: [GlobalISel] Combine (build_vector_trunc x, undef) -> (bitcast x)

Tue Dec 14 06:48:52 PST 2021

foad added inline comments.

================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fcanonicalize.mir:256
     ; CHECK-NEXT: [[FMUL:%[0-9]+]]:_(<2 x s16>) = G_FMUL [[COPY]], %two_splat
+    ; CHECK-NEXT: %zero_undef_fcan:_(<2 x s16>) = G_FCANONICALIZE %zero_undef
     ; CHECK-NEXT: [[FCANONICALIZE:%[0-9]+]]:_(<2 x s16>) = G_FCANONICALIZE [[FMUL]]
----------------
arsenm wrote:
> arsenm wrote:
> > foad wrote:
> > > Regression here because combining `G_BUILD_VECTOR_TRUNC %zero_s32(s32), %undef(s32)` into a G_BITCAST breaks detection of a constant splat with some elements undef.
> > > 
> > > Any thoughts on how to fix this? Perhaps it was not a good idea to do this as a combine after all, despite the comment in AMDGPUInstructionSelector::selectG_BUILD_VECTOR_TRUNC?
> > I think the globalisel version of isCanonicalized needs to be implemented for G_BITCAST
> Or everything really. It's pretty thin right now
I'm not sure having isCanonicalized look through a bitcast from s32 to <2 x s16> is a great idea. And it wouldn't help with the regression in clamp-minmax-const-combine.ll which is to do with recognizing vector splat constants where some elements may be undef.

I see the DAG version of isCanonicalized looks through ISD::BITCAST, but I don't see how that can be safe when it loses the information about whether you were looking at (say) an f32 or a vector of f16s.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112064/new/

https://reviews.llvm.org/D112064