[PATCH] D158256: [RISCV] Fix assertion failure when zcmp extension is enabled.

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

Well for starters you've not even addressed all the feedback from prior reviews, so pinging is a bit premature and rude



================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:1
+; RUN: llc -mattr=+zcmp -verify-machineinstrs  \
+; RUN: -mtriple=riscv32 -target-abi ilp32 < %s \
----------------
jrtc27 wrote:
> Though it looks like the same code is generated for both so can share one prefix?
Use update_llc_test_checks.py


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:1
+; RUN: llc -mattr=+zcmp -verify-machineinstrs  \
+; RUN: -mtriple=riscv32 -target-abi ilp32 < %s \
----------------
craig.topper wrote:
> jrtc27 wrote:
> > jrtc27 wrote:
> > > Though it looks like the same code is generated for both so can share one prefix?
> > Use update_llc_test_checks.py
> Can we decide a single name for the test files? Either frame-lowering-crash.ll/mir or prolog-epilog-crash.mir?
> 
> Maybe worth including zcmp in the test file name too?
Though why is there an IR test and a MIR test? The MIR test seems preferable, surely?


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:1-6
+; RUN: llc -mattr=+zcmp -verify-machineinstrs  \
+; RUN: -mtriple=riscv32 -target-abi ilp32 < %s \
+; RUN: | FileCheck %s -check-prefixes=RV32I
+; RUN: llc -mattr=+zcmp -verify-machineinstrs  \
+; RUN: -mtriple=riscv64 -target-abi lp64 < %s  \
+; RUN: | FileCheck %s -check-prefixes=RV64I
----------------
Though it looks like the same code is generated for both so can share one prefix?


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:14
+declare hidden void @f1() local_unnamed_addr
+define hidden void @f0() local_unnamed_addr {
+; RV32I-LABEL: f0:                                     # @f0
----------------
nounwind


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:14
+declare hidden void @f1() local_unnamed_addr
+define hidden void @f0() local_unnamed_addr {
+; RV32I-LABEL: f0:                                     # @f0
----------------
jrtc27 wrote:
> nounwind
Space between declaration and definition


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:60
+
+if.end:                                           ; preds = %entry
+  br i1 poison, label %if.end3, label %if.then2
----------------
Clean up the IR of these comments?


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:64
+if.then2:                                         ; preds = %if.end
+  tail call void @f1() #4
+  br label %cleanup
----------------
Dangling attribute reference


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:75
+for.body:                                         ; preds = %for.cond
+  store i32 0, ptr null, align 4
+  br label %for.cond
----------------
This seems overly-reduced


================
Comment at: llvm/test/CodeGen/RISCV/frame-lowering-crash.ll:85
+}
\ No newline at end of file

----------------
Fix


================
Comment at: llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir:3-4
+# REQUIRES: asserts
+# RUN: llc  %s -o - -mtriple=riscv32 -mattr=+zcmp -target-abi ilp32 -run-pass=prologepilog \
+# RUN:   -simplify-mir -verify-machineinstrs | FileCheck %s
+
----------------



================
Comment at: llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir:65
+  ; CHECK-NEXT:   $x2 = frame-setup ADDI $x2, -32
+  ; CHECK-NEXT:   frame-setup CFI_INSTRUCTION def_cfa_offset 32
+  ; CHECK-NEXT:   SB $x0, $x2, 31 :: (store (s8) into %stack.0 + 31)
----------------
Get rid of these with nounwind, however that's done in MIR


================
Comment at: llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir:7
+--- |
+  define hidden void @f(fp128 %a) local_unnamed_addr #0 {
+  entry:
----------------
jrtc27 wrote:
> garvitgupta08 wrote:
> > garvitgupta08 wrote:
> > > jrtc27 wrote:
> > > > Is the IR actually needed?
> > > Yes, I think that is how it has been done what other mir unit test cases. Besides it would help to see the .ll file from which the mir testcase is generated.
> > There is a typo - //for// other mir unit testcases.
> Not all MIR tests need it. So I'm asking whether this one specifically does.
You've still not answered this


================
Comment at: llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir:35
+---
+name:            f
+alignment:       2
----------------
jrtc27 wrote:
> garvitgupta08 wrote:
> > jrtc27 wrote:
> > > If you're going to call it `f` then the test needs a comment for what's being tested here
> > It it the reduced testcase through bugpoint which tests the prologepilog pass. 
> So? bugpoint reduces it, but you still need to turn it into a meaningful and readable test.
Still pending


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158256