[Kryo] Use immediate #0 to zero a register

[haicheng]

Summary

Similar to Cyclone, Kryo favors using immediate #0 to zero out registers.

[mcrosier]

I'd replace "HasImmeZeroing" with "UseImmZeroing".

lib/Target/AArch64/AArch64.td

52

s/imme/imm

54

s/great/preferred

56

s/imme/imm

57

s/immez/immz

149

s/Imme/Imm

lib/Target/AArch64/AArch64Subtarget.h

139

s/has/use

[haicheng]

Change one line of the comment.

[haicheng]

Addresses Chad's comments.

[mcrosier]

@t.p.northover: Much like Cyclone, Kryo prefers the move immediate zero idiom for zeroing registers. Unfortunately, when we tried to reuse the ZeroCycleZeroing target feature we found regressions in some of our FP workloads. The issue is described in PR27454. Do you see a way we could have a unified approach for Cyclone and Kryo that avoids the issue in PR27454?

lib/Target/AArch64/AArch64InstrInfo.cpp

1932

If I'm not mistaken, Matt suggested this be Subtarget.isKryo(), rather than adding a new target feature.

[haicheng]

Change to use subtarget hook. Enable float point zeroing as well.

[haicheng]

Now that r274686 is committed, I can simply enable zczeroing feature in Kryo for both integer and float registers. I have to explicitly set the isAsCheapAsAMove feature because Kryo has customized CheapAsMoveHandling.

[MatzeB]

Looks good to me, but I believe the test can be simplified:

test/CodeGen/AArch64/kryo_zeroing.ll

26–45 ↗

(On Diff #62990)

These tests appear to have a lot more instructions than necessary just to produce mov x9, #0; mov x8, #0. Also wouldn't it be better to merge this with arm64-zero-cycle-zeroing.ll, maybe you can just addnother run line there?

[haicheng]

As suggested by Matthias, add another run line in arm64-zero-cycle-zeroing.ll

[MatzeB]

lib/Target/AArch64/AArch64InstrInfo.cpp

629–632

Shouldn't this rather be "case AArch64::COPY: return true"?

The whole purpose of this function is that some passes can decide whether to copy a value around with COPY or just recompute the value instead.

[haicheng]

lib/Target/AArch64/AArch64InstrInfo.cpp

629–632

I agree with you. I don't know why COPY is not included in the first place. However, can I do it in a separate patch because it affects other subtargets? This patch is supposed to affect Kryo only.

[haicheng]

I separate this patch into two parts, ZCZeroing part and isAsCheapAsAMove part. This patch now only has the first part.

I am still collecting performance data of setting any COPY isAsCheapAsAMove on A57 and Kryo which may takes a while. Moreover, I am thinking maybe I should use isZeroCost() API to set all zero cost pseudo-instructions (including COPY) isAsCheapAsAMove. I will upload another patch after the experiment is done.

[MatzeB]

LGTM.

[haicheng]

Thank you, Matthias.

Another thing about Cyclone's ZCZeroing feature is AArch64InstrInfo::isGPRZero() which I believe is only used by Cyclone. This function returns true for COPY WZR but not COPY XZR. I don't know if it is intentional or a mistake.

lib/Target/AArch64/AArch64.td

 def FeatureZCZeroing : SubtargetFeature<"zcz", "HasZeroCycleZeroing", "true",
                                         "Has zero-cycle zeroing instructions">;
 
+/// Similar to Cyclone, Kryo favors using imme #0 to zero out registers.  But
+/// Cylone's approach to zero a D or S register (movi D.2d + extract subreg) is
+/// not great in Kryo.
+def FeatureImmeZeroing : SubtargetFeature<"immez", "HasImmeZeroing", "true",
+                                          "Use Immediate #0 to zero registers">;
+
 def FeatureStrictAlign : SubtargetFeature<"strict-align",
                                           "StrictAlign", "true",
                                           "Disallow all unaligned memory "

                                    FeatureNEON,
                                    FeatureCrypto,
                                    FeatureCRC,
-                                   FeaturePerfMon]>;
+                                   FeaturePerfMon,
+                                   FeatureImmeZeroing]>;
 
 def : ProcessorModel<"generic", NoSchedModel, [FeatureFPARMv8,
                                               FeatureNEON,

lib/Target/AArch64/AArch64InstrInfo.cpp

             .addImm(0)
             .addImm(AArch64_AM::getShifterImm(AArch64_AM::LSL, 0));
       }
-    } else if (SrcReg == AArch64::WZR && Subtarget.hasZeroCycleZeroing()) {
+    } else if (SrcReg == AArch64::WZR && (Subtarget.hasZeroCycleZeroing() ||
+                                          Subtarget.hasImmeZeroing())) {
       BuildMI(MBB, I, DL, get(AArch64::MOVZWi), DestReg).addImm(0).addImm(
           AArch64_AM::getShifterImm(AArch64_AM::LSL, 0));
     } else {

           .addReg(SrcReg, getKillRegState(KillSrc))
           .addImm(0)
           .addImm(AArch64_AM::getShifterImm(AArch64_AM::LSL, 0));
-    } else if (SrcReg == AArch64::XZR && Subtarget.hasZeroCycleZeroing()) {
+    } else if (SrcReg == AArch64::XZR && (Subtarget.hasZeroCycleZeroing() ||
+                                          Subtarget.hasImmeZeroing())) {
       BuildMI(MBB, I, DL, get(AArch64::MOVZXi), DestReg).addImm(0).addImm(
           AArch64_AM::getShifterImm(AArch64_AM::LSL, 0));
     } else {

lib/Target/AArch64/AArch64Subtarget.h

   // HasZeroCycleZeroing - Has zero-cycle zeroing instructions.
   bool HasZeroCycleZeroing;
 
+  // HasImmeZeroing - Use Immediate #0 to zero a register.
+  bool HasImmeZeroing;
+
   // StrictAlign - Disallow unaligned memory accesses.
   bool StrictAlign;
 

 
   bool hasZeroCycleZeroing() const { return HasZeroCycleZeroing; }
 
+  bool hasImmeZeroing() const { return HasImmeZeroing; }
+
   bool requiresStrictAlign() const { return StrictAlign; }
 
   bool isX18Reserved() const { return ReserveX18; }

lib/Target/AArch64/AArch64Subtarget.cpp

       HasV8_1aOps(false), HasV8_2aOps(false), HasFPARMv8(false), HasNEON(false),
       HasCrypto(false), HasCRC(false), HasPerfMon(false), HasFullFP16(false),
       HasZeroCycleRegMove(false), HasZeroCycleZeroing(false),
-      StrictAlign(false), ReserveX18(TT.isOSDarwin()), IsLittle(LittleEndian),
-      CPUString(CPU), TargetTriple(TT), FrameLowering(),
+      HasImmeZeroing(false), StrictAlign(false), ReserveX18(TT.isOSDarwin()),
+      IsLittle(LittleEndian), CPUString(CPU), TargetTriple(TT), FrameLowering(),
       InstrInfo(initializeSubtargetDependencies(FS)), TSInfo(),
       TLInfo(TM, *this), GISel() {}
 

test/CodeGen/AArch64/imme_zeroing.ll

+; RUN: llc -mtriple=aarch64-gnu-linux -mcpu=kryo < %s | FileCheck %s
+
+; Verify that immediate #0 is used when zeroing a register.
+ 
+define i32 @test_int32(i32* nocapture readonly %p) {
+; CHECK-LABEL:  test_int32:
+; CHECK:  movz   x9, #0
+; CHECK:  movz   w8, #0
+entry:
+  br label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.body
+  ret i32 %add
+
+for.body:                                         ; preds = %for.body, %entry
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %sum.06 = phi i32 [ 0, %entry ], [ %add, %for.body ]
+  %arrayidx = getelementptr inbounds i32, i32* %p, i64 %indvars.iv
+  %0 = load i32, i32* %arrayidx, align 4
+  %add = add nsw i32 %0, %sum.06
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond = icmp eq i64 %indvars.iv.next, 10
+  br i1 %exitcond, label %for.cond.cleanup, label %for.body
+}
+
+define i64 @test_int64(i64* nocapture readonly %p) {
+; CHECK-LABEL:  test_int64:
+; CHECK:  movz   x9, #0
+; CHECK:  movz   x8, #0
+entry:
+  br label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.body
+  ret i64 %add
+
+for.body:                                         ; preds = %for.body, %entry
+  %i.07 = phi i64 [ 0, %entry ], [ %inc, %for.body ]
+  %sum.06 = phi i64 [ 0, %entry ], [ %add, %for.body ]
+  %arrayidx = getelementptr inbounds i64, i64* %p, i64 %i.07
+  %0 = load i64, i64* %arrayidx, align 8
+  %add = add nsw i64 %0, %sum.06
+  %inc = add nuw nsw i64 %i.07, 1
+  %exitcond = icmp eq i64 %inc, 10
+  br i1 %exitcond, label %for.cond.cleanup, label %for.body
+}