[ARM] Fix Chain/Glue Bug in PerformVMOVhrCombine

[lenary]

Summary

In this optimisation, the Chain and Glue from the original CopyFromReg
was being lost by this optimisation, which resulted in miscompiles.

This fix just ensures that the input chains are correctly updated, and
that any any users are also updated with the new chain from the new
CopyFromReg.

Fixes #60510.

[dmgreen]

Sounds OK if you can clear up the tests. It might be worth making sure there is a test with multiple uses too.

llvm/lib/Target/ARM/ARMISelLowering.cpp

15005

Can remove the extra t18: f16 = ARMISD::VMOVhr t5?

[lenary]

In D143713#4122025, @dmgreen wrote:

Sounds OK if you can clear up the tests. It might be worth making sure there is a test with multiple uses too.

Do you mean the AES pass tests? Do you think that should happen before we land this, they've been an issue for a little while.

llvm/lib/Target/ARM/ARMISelLowering.cpp

15005

Ack

[dmgreen]

Do you mean the AES pass tests? Do you think that should happen before we land this, they've been an issue for a little while.

I think I just meant https://reviews.llvm.org/D143712#4117617 for "clear up the tests". A Multiple uses test would be useful to check that nothing odd goes on when the returned value is used elsewhere too.

[lenary]

In D143713#4139346, @dmgreen wrote:

Do you mean the AES pass tests? Do you think that should happen before we land this, they've been an issue for a little while.

I think I just meant https://reviews.llvm.org/D143712#4117617 for "clear up the tests". A Multiple uses test would be useful to check that nothing odd goes on when the returned value is used elsewhere too.

Ah, right. Sorry, that leaked out my memory. Yes, I do intend to do a multiple uses test as well.

[lenary]

Tests and comment updated. I also split this patch stack away from the unrelated changes for inline asm handling of fp16/bf16.

[dmgreen]

Thanks. LGTM

llvm/lib/Target/ARM/ARMISelLowering.cpp

static SDValue PerformVMOVhrCombine(SDNode *N,
 
   // VMOVhr (VMOVrh (X)) -> X
   if (Op0->getOpcode() == ARMISD::VMOVrh)
     return Op0->getOperand(0);
 
   // FullFP16: half values are passed in S-registers, and we don't
   // need any of the bitcast and moves:
   //
-  //     t2: f32,ch = CopyFromReg t0, Register:f32 %0
+  //     t2: f32,ch1,gl1? = CopyFromReg ch, Register:f32 %0, gl?
   //   t5: i32 = bitcast t2
   // t18: f16 = ARMISD::VMOVhr t5
+  // =>
+  // tN: f16,ch2,gl2? = CopyFromReg ch, Register::f32 %0, gl?
   if (Op0->getOpcode() == ISD::BITCAST) {
     SDValue Copy = Op0->getOperand(0);
     if (Copy.getValueType() == MVT::f32 &&
         Copy->getOpcode() == ISD::CopyFromReg) {
-      SDValue Ops[] = {Copy->getOperand(0), Copy->getOperand(1)};
+      bool HasGlue = Copy->getNumOperands() == 3;
+      SDValue Ops[] = {Copy->getOperand(0), Copy->getOperand(1),
+                       HasGlue ? Copy->getOperand(2) : SDValue()};
+      EVT OutTys[] = {N->getValueType(0), MVT::Other, MVT::Glue};
       SDValue NewCopy =
-          DCI.DAG.getNode(ISD::CopyFromReg, SDLoc(N), N->getValueType(0), Ops);
+          DCI.DAG.getNode(ISD::CopyFromReg, SDLoc(N),
+                          DCI.DAG.getVTList(ArrayRef(OutTys, HasGlue ? 3 : 2)),
+                          ArrayRef(Ops, HasGlue ? 3 : 2));
+
+      // Update Users, Chains, and Potential Glue.
+      DCI.DAG.ReplaceAllUsesOfValueWith(SDValue(N, 0), NewCopy.getValue(0));
+      DCI.DAG.ReplaceAllUsesOfValueWith(Copy.getValue(1), NewCopy.getValue(1));
+      if (HasGlue)
+        DCI.DAG.ReplaceAllUsesOfValueWith(Copy.getValue(2),
+                                          NewCopy.getValue(2));
+
       return NewCopy;
     }
   }
 
   // fold (VMOVhr (load x)) -> (load (f16*)x)
   if (LoadSDNode *LN0 = dyn_cast<LoadSDNode>(Op0)) {
     if (LN0->hasOneUse() && LN0->isUnindexed() &&
         LN0->getMemoryVT() == MVT::i16) {

llvm/test/CodeGen/ARM/fp16-return-pr60510.ll

 ; FP16-SOFT:    bl other
 ; FP16-SOFT:    vmov.f16 s0, r5
 ; FP16-SOFT:    vmov r0, s0
 ;
 ; FP16-HARD-LABEL: fp16_out_call_oneuse:
 ; FP16-HARD:  @ %bb.0:
 ; FP16-HARD:    vmov.f32 s16, s0
 ; FP16-HARD:    bl fp16_inner
+; FP16-HARD:    vmov.f32 s18, s0
 ; FP16-HARD:    vmov.f32 s0, s16
 ; FP16-HARD:    bl other
+; FP16-HARD:    vmov.f32 s0, s18
   %call = call half @fp16_inner()
   %call1 = call float @other(float %arg)
   ret half %call
 }
 
 define half @fp16_out_call_multiuse(float %arg) nounwind {
 ; NO-FP16-SOFT-LABEL: fp16_out_call_multiuse:
 ; NO-FP16-SOFT:  @ %bb.0:

 ; FP16-SOFT:    vmov.f16 r0, s16
 ; FP16-SOFT:    bl fp16_sink
 ; FP16-SOFT:    vmov r0, s16
 ;
 ; FP16-HARD-LABEL: fp16_out_call_multiuse:
 ; FP16-HARD:  @ %bb.0:
 ; FP16-HARD:    vmov.f32 s16, s0
 ; FP16-HARD:    bl fp16_inner
+; FP16-HARD:    vmov.f32 s18, s0
 ; FP16-HARD:    vmov.f32 s0, s16
 ; FP16-HARD:    bl other
-; FP16-HARD:    vmov.f16 r0, s0
-; FP16-HARD:    vmov.f32 s16, s0
+; FP16-HARD:    vmov.f16 r0, s18
 ; FP16-HARD:    vmov s0, r0
 ; FP16-HARD:    bl fp16_sink
-; FP16-HARD:    vmov.f32 s0, s16
+; FP16-HARD:    vmov.f32 s0, s18
   %call = call half @fp16_inner()
   %call1 = call float @other(float %arg)
   call void @fp16_sink(half %call)
   ret half %call
 }
 
 define bfloat @bf_out_call_oneuse(float %arg) nounwind {
 ; NO-FP16-SOFT-LABEL: bf_out_call_oneuse: