[PATCH] D67061: [mir-canon][NFCi] Adding opt arg to enable vreg renaming only mode.

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 3 inline comments as done.
plotfi added inline comments.


================
Comment at: llvm/test/CodeGen/MIR/AArch64/mirCanonCopyCopyProp.mir:1-2
-# RUN: llc -mtriple=arm64-apple-ios11.0.0 -o - -run-pass mir-canonicalizer %s | FileCheck %s
+# RUN: llc -mtriple=arm64-apple-ios11.0.0 -o - -run-pass mir-canonicalizer %s | FileCheck --check-prefixes=CHECK-FULL %s
+# RUN: llc -mtriple=arm64-apple-ios11.0.0 -o - -run-pass mir-canonicalizer -canon-vregs-namer=1 %s | FileCheck --check-prefixes=CHECK-NAMER %s
+
----------------
paquette wrote:
> Even though this test is pretty small, I think it would be better to have an explicit separate test for this.
> 
> (Then maybe you can use llvm/utils/update_mir_test_checks.py too, which is a nice time-saver. Not sure if it would work on this though?)
> 
> Also, is there any particular reason you don't pass -verify-machineinstrs on these tests? IIRC it always kicks in on debug builds, so it's nice to just always have it to prevent bot failures.
actually, if I am going to do a new pass then year a separate test makes more sense.

I dont think there is a good reason the verifier isnt enabled here. I will add it. 


================
Comment at: llvm/test/CodeGen/MIR/AArch64/mirCanonIdempotent.mir:11-19
+# CHECK-NAMER: %namedVReg1361:fpr64 = COPY $d1
+# CHECK-NAMER-NEXT: %namedVReg1358:fpr64 = COPY $d0
+# CHECK-NAMER-NEXT: %namedVReg1355:gpr64 = COPY $x1
+# CHECK-NAMER-NEXT: %namedVReg1352:gpr64common = COPY $x0
+# CHECK-NAMER-NEXT: STRXui %namedVReg1352, %stack.1, 0 :: (store 8)
+# CHECK-NAMER-NEXT: STRXui %namedVReg1355, %stack.2, 0 :: (store 8)
+# CHECK-NAMER-NEXT: STRDui %namedVReg1358, %stack.3, 0 :: (store 8)
----------------
paquette wrote:
> I think this should be split out into a separate test as well.
> 
> Also, in the separate test, it looks like you can just axe everything beyond the FMOVDi since that's all that's tested anyway.
> 
> (Is there any particular reason this test is so long in the first place?)
The reason it's so big I dont think is a very good one. I think I was just looking for some MIR code that had a ton of instructions like .* =  FMOVDi 20 that could be moved to the top and sorted alphabetically. 


================
Comment at: llvm/test/CodeGen/MIR/AArch64/mirCanonVRegnamer.mir:7-12
+--- |
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64-unknown-linux-gnu"
+  define void @foo() {
+    ret void
+  }
----------------
paquette wrote:
> I don't think you need the IR if you pass in `-mtriple aarch64-unknown-unknown` to the llc line.
Noted. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67061