[PATCH] D66467: [Codegen] skip debug instr to avoid code change

Mon Sep 2 08:59:03 PDT 2019

jmorse added a comment.

Looking good; a couple of extra nits, then it's ready to go.

I think the CHECK lines need strengthening a little, by changing for example:

  ; CHECK: bb.5:
  ; CHECK: successors: %bb.6(0x80000000)  

to

  ; CHECK: bb.5:
  ; CHECK-NEXT: successors: %bb.6(0x80000000)  

Otherwise, FileCheck could in theory accept any number of lines between "bb.5:" and the successor list, including other blocks. Due to the way that the test is written that probably wouldn't be a problem right now, but tests get updated all the time, and explicitly connecting pairs of CHECK lines with -NEXT will make it harder for a future developer to make a mistake when editing this test.

IMO it'd be best to replace the "0x80000000" ... etc constants in the successors list with a regex (i.e. "{{0x[0-9]+}}") to avoid testing too much. These are branch frequency weights that aren't important for this test.

Ideally there would also be a comment in the test file explaining what should happen -- i.e. "Branch folder should ignore the DBG_VALUE between [...] and merge these blocks", or whatever it's supposed to be doing.

================
Comment at: llvm/test/CodeGen/MIR/X86/branch-folder-with-debug.mir:45
+
+# RUN: llc -o - %s -mtriple=x86_64-- -run-pass=branch-folder | FileCheck %s
+--- |
----------------
NB, I believe the convention is to put the RUN line at the very top of the file -- I don't think it's rule, but it's best to avoid surprise.

================
Comment at: llvm/test/CodeGen/MIR/X86/branch-folder-with-debug.mir:57
+  bb.0:
+    ; CHECK: successors: %bb.1(0x40000000), %bb.7(0x40000000)
+    successors: %bb.1, %bb.3
----------------
If this successor list is going to be tested for, the block it's in should be tested too. Also on line 64.

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

https://reviews.llvm.org/D66467