[PATCH] D72753: [ARM] Fixup FP16 bitcasts

[Dave Green via Phabricator via llvm-commits]

dmgreen marked an inline comment as done.
dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5720
          Op.getValueType() != MVT::f32)
-       return SDValue();
+       return SDValue(N, 0);
 
----------------
efriedma wrote:
> dmgreen wrote:
> > efriedma wrote:
> > > Can we make this optimization a TargetDAGCombine, instead of wedging it into legalization?
> > Can you explain what you mean?
> > 
> > My understanding is that this is where the bug is. That we shouldn't be returning expand (SDValue()) if the node is actually legal. Trying to add something that sounds like custom bitcast lowering doesn't sound right to me.
> > 
> > But I may be wrong about any or all of that. Let me know.
> An f32<->i32 bitcast is always legal; it shouldn't be marked "Custom" in the first place.
> 
> The transform here is to transform `VMOVhr(bitcast(CopyFromReg)) -> CopyFromReg`.  That seems like a perfect candidate for a TargetDAGCombine.
Ah. Rewrite the existing code? Sure, that sounds like something that should work. And like you said might be cleaner. Let me give it a go.


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

https://reviews.llvm.org/D72753