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

[garvitgupta08]

Summary

Before accessing "getOpcode" thorugh machine instruction, check if the iterator
has reached the end of Machine basic block otherwise we will crash at the
assertion !NodePtr->isKnownSentinel().

The above assertion is hit in "Prologue/Epilogue Insertion & Frame Finalization
pass".

[jrtc27]

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

2 ↗

(On Diff #551414)

REQUIRES

3 ↗

(On Diff #551414)

4 ↗

(On Diff #551414)

Don't need a custom CHECK prefix when there's just one in the whole file (and if you only have one for that FileCheck invocation you should use --check-prefix). Besides, what's IPE?..

7 ↗

(On Diff #551414)

Is the IR actually needed?

35 ↗

(On Diff #551414)

If you're going to call it f then the test needs a comment for what's being tested here

[garvitgupta08]

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

7 ↗

(On Diff #551414)

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.

35 ↗

(On Diff #551414)

It it the reduced testcase through bugpoint which tests the prologepilog pass.

[garvitgupta08]

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

7 ↗

(On Diff #551414)

There is a typo - for other mir unit testcases.

[jrtc27]

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

7 ↗

(On Diff #551414)

Not all MIR tests need it. So I'm asking whether this one specifically does.

35 ↗

(On Diff #551414)

So? bugpoint reduces it, but you still need to turn it into a meaningful and readable test.

[craig.topper]

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

31 ↗

(On Diff #551450)

This attribute shouldn't be needed since it is on the command line.

[garvitgupta08]

When enabling zcmp extension to build compiler-rt, fixunstfsi.c was crashing at the above mentioned pass. The testcase used is reduced through bugpoint from the generated llvm IR from fixunstfsi.c source.

[anmolparalkar-nxp]

@garvitgupta08 I have closed D158379 after starting with your patch from this (D158256) and merging in llvm/test/CodeGen/RISCV/frame-lowering-crash.ll
Kindly download the patch from D158379 as the starting point prior to your next upload to this (D158256). Thank you!
@wangpc thanks for the advice.

[garvitgupta08]

Thanks @anmolparalkar-nxp for informing. Merged the change from D158379.

[anmolparalkar-nxp]

@garvitgupta08 Once committed to main, please do file an issue to add the fix to the 17.0.x milestone, for it to make it into LLVM 17.0.0-RC4 Thank you!

[garvitgupta08]

Hi @craig.topper @jrtc27, do let me know if there are any further changes needed. If the current patch is fine, please commit it on my behalf; below are my credentials
Name - Garvit Gupta
Username - garvitgupta08
email id - [e-mail]

[craig.topper]

llvm/test/CodeGen/RISCV/frame-lowering-crash.ll

1 ↗

(On Diff #552770)

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?

13 ↗

(On Diff #552770)

Do we need the hidden and local_unnamed_addr attributes?

[jrtc27]

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

llvm/test/CodeGen/RISCV/frame-lowering-crash.ll

1 ↗

(On Diff #552770)

Use update_llc_test_checks.py

1 ↗

(On Diff #552770)

Though why is there an IR test and a MIR test? The MIR test seems preferable, surely?

1–6 ↗

(On Diff #552770)

Though it looks like the same code is generated for both so can share one prefix?

14 ↗

(On Diff #552770)

nounwind

14 ↗

(On Diff #552770)

Space between declaration and definition

60 ↗

(On Diff #552770)

Clean up the IR of these comments?

64 ↗

(On Diff #552770)

Dangling attribute reference

75 ↗

(On Diff #552770)

This seems overly-reduced

85 ↗

(On Diff #552770)

Fix

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

3–4 ↗

(On Diff #552770)

65 ↗

(On Diff #552770)

Get rid of these with nounwind, however that's done in MIR

7 ↗

(On Diff #551414)

You've still not answered this

35 ↗

(On Diff #551414)

Still pending

[garvitgupta08]

@anmolparalkar-nxp can you please take a look at the comments on zcmp-prolog-epilog-crash.ll test?

[garvitgupta08]

In D158256#4605968, @garvitgupta08 wrote:

When enabling zcmp extension to build compiler-rt, fixunstfsi.c was crashing at the above mentioned pass. The testcase used is reduced through bugpoint from the generated llvm IR from fixunstfsi.c source.

@jrtc27 I commented about the source of the IR used in MIR testcase. I didn't hear back on it so I thought the test currently is fine. Let me know if I should put the original IR in the testcase without reducing it through bugpoint so that it can more meaningful and readable.

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

7 ↗

(On Diff #551414)

The only reason for me to include IR in the testcase is to know the source from which the Machine instructions are generated. If it does not help much in this case, I can remove it.

[anmolparalkar-nxp]

In D158256#4612778, @garvitgupta08 wrote:

@anmolparalkar-nxp can you please take a look at the comments on zcmp-prolog-epilog-crash.ll test?

@garvitgupta08 I will start with your current diff from here, address the comments, and upload the new patch to D158379 and ping you; following which, please start by downloading and continuing from there ...
Also, please feel free to drop it (IR test) in favour of the more preferred MIR test ...

[anmolparalkar-nxp]

llvm/test/CodeGen/RISCV/frame-lowering-crash.ll

75 ↗

(On Diff #552770)

@jrtc27 Kindly clarify (overly-reduced) I'll include matter as necessary, thanks.

[craig.topper]

llvm/test/CodeGen/RISCV/frame-lowering-crash.ll

75 ↗

(On Diff #552770)

icmp slt i32 poison, poison doesn't make sense. The compiler would be allowed to delete it. So a future optimization could break this test.

store i32 0, ptr null, align 4 is a store to null which is undefined behavior. So the compiler is allowed to delete it. Again a future optimization could delete it and break this test.

[Jim]

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

15 ↗

(On Diff #553000)

Does it have any difference of codegen between RV32 and RV64? Can we use the same check-prefixes?

[anmolparalkar-nxp]

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

15 ↗

(On Diff #553000)

No difference in codegen between RV32 and RV64; yes, agreed on the same check-prefixes. Thanks.

[anmolparalkar-nxp]

llvm/test/CodeGen/RISCV/frame-lowering-crash.ll

75 ↗

(On Diff #552770)

icmp slt i32 poison, poison doesn't make sense. The compiler would be allowed to delete it. So a future optimization could break this test.

Agreed, I can rewrite without the use of poison (here and elsewhere) and preserve reproducibility.

store i32 0, ptr null, align 4 is a store to null which is undefined behavior. So the compiler is allowed to delete it. Again a future optimization could delete it and break this test.

• however, I am unable to reproduce the crash if I replace the 'ptr null' with a regular address. Any suggestions? Thanks.

[anmolparalkar-nxp]

llvm/test/CodeGen/RISCV/frame-lowering-crash.ll

75 ↗

(On Diff #552770)

store i32 0, ptr null, align 4 is a store to null which is undefined behavior. So the compiler is allowed to delete it. Again a future optimization could delete it and break this test.

• however, I am unable to reproduce the crash if I replace the 'ptr null' with a regular address. Any suggestions? Thanks.

ah, never mind; I am able to reproduce the crash by replacing the 'ptr null' with:

store i32 0, ptr inttoptr (i32 -559038737 to ptr), align 4

[anmolparalkar-nxp]

@garvitgupta08 Please download updated patch from D158379 with review comments to llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll addressed
I have included a checklist of the comments from here, along with their resolution for you to be able to verify and check each one off. Thanks so much!

[garvitgupta08]

Addressed review comments to llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll from D158256:

Do we need the hidden and local_unnamed_addr attributes?
DONE

icmp slt i32 poison, poison doesn't make sense. The compiler would be allowed to delete it. So a future optimization could break this test.
DONE

store i32 0, ptr null, align 4 is a store to null which is undefined behavior. So the compiler is allowed to delete it. Again a future optimization could delete it and break this test.
DONE

Use update_llc_test_checks.py
DONE

Though why is there an IR test and a MIR test? The MIR test seems preferable, surely?
Please feel free to drop: llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

Though it looks like the same code is generated for both so can share one prefix?
Actually, there is a difference ...

nounwind
DONE

Space between declaration and definition
DONE

Clean up the IR of these comments?
DONE

Dangling attribute reference
DONE

This seems overly-reduced
Asked for clarification ...

Fix // No newline at end of file
DONE?

[anmolparalkar-nxp]

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

15 ↗

(On Diff #553000)

After incorporating @craig.topper's comments on the use of poison value and the write to 'ptr null' being code fragments that could potentially be optimized away in the future, I respun the code and now the code generated is different for RV32 and RV64; the checks themselves being auto-generated by utils/update_llc_test_checks.py

[anmolparalkar-nxp]

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

15 ↗

(On Diff #553000)

No longer applicable; please see note right below for the clarification. Thanks.

[anmolparalkar-nxp]

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

15 ↗

(On Diff #553000)

@garvitgupta08 Please mark both as Done (actually, this is no longer applicable, as there _is_ a difference between the RV32 code and RV64 code - and the check-prefixes are automatically generated by utils/update_llc_test_checks.py). Please change the state from Not Done to Done on both @Jim's and @anmolparalkar-nxp's comments, thank you.

[jrtc27]

Once again, since I can’t obviously see an answer to it: why are there both MIR and IR tests?

[anmolparalkar-nxp]

@jrtc27 - to answer your question, the .ll test came in per @wangpc's advice in D158379 to merge the test from there, in here; I incorporated all the comments on the .ll test, made here, to make myself more educated in the methodology to be followed, in general, and of-course, to respect the review feedback that was given for the .ll, here. That said, I have already clarified on August 24th: "Please feel free to drop: llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll" Thank you.

@garvitgupta08, It is most important that this fix gets included in the 17.0.x milestone, hopefully, it makes it in time for LLVM 17.0.0-RC4 Thanks!

[garvitgupta08]

I would like to merge this patch as soon as possible so that it can be merged into community release 17.0. Do let me know if there is really a need to have only MIR test, otherwise I think that all comments have been addressed.

[craig.topper]

In D158256#4625476, @garvitgupta08 wrote:

I would like to merge this patch as soon as possible so that it can be merged into community release 17.0. Do let me know if there is really a need to have only MIR test, otherwise I think that all comments have been addressed.

Drop the IR test. We don't need two tests for the same issue.

[garvitgupta08]

Addressed the comments

[craig.topper]

LGTM

[wangpc]

LGTM.

[garvitgupta08]

Please commit it on my behalf; below are my credentials
Name - Garvit Gupta
Username - garvitgupta08
email id - [e-mail]

[craig.topper]

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.mir

3

Why does the test require asserts?

[garvitgupta08]

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.mir

3

The assertion is hit only when asserts are enabled. With release build with no assert, it is able to generate some machine instructions

[jrtc27]

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

3–4 ↗

(On Diff #552770)

This wasn't addressed

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

if (FirstSPAdjustAmount) {
     RI->adjustReg(MBB, LastFrameDestroy, DL, SPReg, SPReg,
                   StackOffset::getFixed(SecondSPAdjustAmount),
                   MachineInstr::FrameDestroy, getStackAlign());
   }
 
   if (FirstSPAdjustAmount)
     StackSize = FirstSPAdjustAmount;
 
-  if (RVFI->isPushable(MF) && MBBI->getOpcode() == RISCV::CM_POP) {
+  if (RVFI->isPushable(MF) && MBBI != MBB.end() &&
+      MBBI->getOpcode() == RISCV::CM_POP) {
     // Use available stack adjustment in pop instruction to deallocate stack
     // space.
     unsigned PushStack = RVFI->getRVPushRegs() * (STI.getXLen() / 8);
     unsigned SpImmBase = RVFI->getRVPushStackSize();
     StackSize = adjSPInPushPop(MBBI, StackSize, (SpImmBase - PushStack), true);
   }
 
   // Deallocate stack

llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
+# REQUIRES: asserts
+# RUN: llc  %s -o - -mtriple=riscv32 -mattr=+zcmp -target-abi ilp32 -run-pass=prologepilog \
+# RUN:   -simplify-mir -verify-machineinstrs | FileCheck %s
+
+--- |
+  define hidden void @f(fp128 %a) local_unnamed_addr #0 {
+  entry:
+    %0 = bitcast fp128 %a to i128
+    %and.i = lshr i128 %0, 112
+    %1 = trunc i128 %and.i to i32
+    %2 = and i32 %1, 32767
+    %or.i = or i128 poison, 5192296858534827628530496329220096
+    br label %if.end.i
+
+  if.end.i:                                         ; preds = %entry
+    br i1 poison, label %exit, label %if.then12.i
+
+  if.then12.i:                                      ; preds = %if.end.i
+    %sub13.i = sub nuw nsw i32 16495, %2
+    %sh_prom.i = zext i32 %sub13.i to i128
+    %shr14.i = lshr i128 %or.i, %sh_prom.i
+    %conv15.i = trunc i128 %shr14.i to i32
+    br label %exit
+
+  exit:                                             ; preds = %if.then12.i, %if.end.i
+    %retval.0.i = phi i32 [ %conv15.i, %if.then12.i ], [ -1, %if.end.i ]
+    ret void
+  }
+...
+---
+name:            f
+alignment:       2
+tracksRegLiveness: true
+tracksDebugUserValues: true
+liveins:
+  - { reg: '$x10' }
+frameInfo:
+  maxAlignment:    1
+  localFrameSize:  32
+  savePoint:       '%bb.2'
+  restorePoint:    '%bb.2'
+stack:
+  - { id: 0, size: 32, alignment: 1, local-offset: -32 }
+machineFunctionInfo:
+  varArgsFrameIndex: 0
+  varArgsSaveSize: 0
+body:             |
+  ; CHECK-LABEL: name: f
+  ; CHECK: bb.0.entry:
+  ; CHECK-NEXT:   liveins: $x10
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   renamable $x10 = ADDI $x0, -1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1.if.end.i:
+  ; CHECK-NEXT:   liveins: $x10
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   BNE $x0, $x0, %bb.3
+  ; CHECK-NEXT:   PseudoBR %bb.2
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2.if.then12.i:
+  ; CHECK-NEXT:   liveins: $x10
+  ; CHECK-NEXT: {{  $}}
+  ; 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)
+  ; CHECK-NEXT:   SB $x0, $x2, 30 :: (store (s8) into %stack.0 + 30)
+  ; CHECK-NEXT:   SB $x0, $x2, 29 :: (store (s8) into %stack.0 + 29)
+  ; CHECK-NEXT:   SB $x0, $x2, 28 :: (store (s8) into %stack.0 + 28)
+  ; CHECK-NEXT:   SB $x0, $x2, 27 :: (store (s8) into %stack.0 + 27)
+  ; CHECK-NEXT:   SB $x0, $x2, 26 :: (store (s8) into %stack.0 + 26)
+  ; CHECK-NEXT:   SB $x0, $x2, 25 :: (store (s8) into %stack.0 + 25)
+  ; CHECK-NEXT:   SB $x0, $x2, 24 :: (store (s8) into %stack.0 + 24)
+  ; CHECK-NEXT:   SB $x0, $x2, 23 :: (store (s8) into %stack.0 + 23)
+  ; CHECK-NEXT:   SB $x0, $x2, 22 :: (store (s8) into %stack.0 + 22)
+  ; CHECK-NEXT:   SB $x0, $x2, 21 :: (store (s8) into %stack.0 + 21)
+  ; CHECK-NEXT:   SB $x0, $x2, 20 :: (store (s8) into %stack.0 + 20)
+  ; CHECK-NEXT:   SB $x0, $x2, 19 :: (store (s8) into %stack.0 + 19)
+  ; CHECK-NEXT:   SB $x0, $x2, 18 :: (store (s8) into %stack.0 + 18)
+  ; CHECK-NEXT:   SB $x0, $x2, 17 :: (store (s8) into %stack.0 + 17)
+  ; CHECK-NEXT:   SB $x0, $x2, 16 :: (store (s8) into %stack.0 + 16)
+  ; CHECK-NEXT:   SB renamable $x10, $x2, 0 :: (store (s8) into %stack.0)
+  ; CHECK-NEXT:   SB renamable $x10, $x2, 4 :: (store (s8) into %stack.0 + 4)
+  ; CHECK-NEXT:   renamable $x11 = SRLI renamable $x10, 24
+  ; CHECK-NEXT:   SB renamable $x11, $x2, 3 :: (store (s8) into %stack.0 + 3)
+  ; CHECK-NEXT:   renamable $x12 = SRLI renamable $x10, 16
+  ; CHECK-NEXT:   SB renamable $x12, $x2, 2 :: (store (s8) into %stack.0 + 2)
+  ; CHECK-NEXT:   renamable $x13 = SRLI renamable $x10, 8
+  ; CHECK-NEXT:   SB renamable $x13, $x2, 1 :: (store (s8) into %stack.0 + 1)
+  ; CHECK-NEXT:   SB renamable $x10, $x2, 8 :: (store (s8) into %stack.0 + 8)
+  ; CHECK-NEXT:   SB renamable $x11, $x2, 7 :: (store (s8) into %stack.0 + 7)
+  ; CHECK-NEXT:   SB renamable $x12, $x2, 6 :: (store (s8) into %stack.0 + 6)
+  ; CHECK-NEXT:   SB renamable $x13, $x2, 5 :: (store (s8) into %stack.0 + 5)
+  ; CHECK-NEXT:   SB killed renamable $x10, $x2, 12 :: (store (s8) into %stack.0 + 12)
+  ; CHECK-NEXT:   SB renamable $x11, $x2, 11 :: (store (s8) into %stack.0 + 11)
+  ; CHECK-NEXT:   SB renamable $x12, $x2, 10 :: (store (s8) into %stack.0 + 10)
+  ; CHECK-NEXT:   SB renamable $x13, $x2, 9 :: (store (s8) into %stack.0 + 9)
+  ; CHECK-NEXT:   SB killed renamable $x11, $x2, 15 :: (store (s8) into %stack.0 + 15)
+  ; CHECK-NEXT:   SB killed renamable $x12, $x2, 14 :: (store (s8) into %stack.0 + 14)
+  ; CHECK-NEXT:   SB killed renamable $x13, $x2, 13 :: (store (s8) into %stack.0 + 13)
+  ; CHECK-NEXT:   $x2 = frame-destroy ADDI $x2, 32
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.3.exit:
+  ; CHECK-NEXT:   PseudoRET
+  bb.0.entry:
+    liveins: $x10
+
+    renamable $x10 = ADDI $x0, -1
+
+  bb.1.if.end.i:
+    liveins: $x10
+
+    BNE $x0, $x0, %bb.3
+    PseudoBR %bb.2
+
+  bb.2.if.then12.i:
+    liveins: $x10
+
+    SB $x0, %stack.0, 31 :: (store (s8) into %stack.0 + 31)
+    SB $x0, %stack.0, 30 :: (store (s8) into %stack.0 + 30)
+    SB $x0, %stack.0, 29 :: (store (s8) into %stack.0 + 29)
+    SB $x0, %stack.0, 28 :: (store (s8) into %stack.0 + 28)
+    SB $x0, %stack.0, 27 :: (store (s8) into %stack.0 + 27)
+    SB $x0, %stack.0, 26 :: (store (s8) into %stack.0 + 26)
+    SB $x0, %stack.0, 25 :: (store (s8) into %stack.0 + 25)
+    SB $x0, %stack.0, 24 :: (store (s8) into %stack.0 + 24)
+    SB $x0, %stack.0, 23 :: (store (s8) into %stack.0 + 23)
+    SB $x0, %stack.0, 22 :: (store (s8) into %stack.0 + 22)
+    SB $x0, %stack.0, 21 :: (store (s8) into %stack.0 + 21)
+    SB $x0, %stack.0, 20 :: (store (s8) into %stack.0 + 20)
+    SB $x0, %stack.0, 19 :: (store (s8) into %stack.0 + 19)
+    SB $x0, %stack.0, 18 :: (store (s8) into %stack.0 + 18)
+    SB $x0, %stack.0, 17 :: (store (s8) into %stack.0 + 17)
+    SB $x0, %stack.0, 16 :: (store (s8) into %stack.0 + 16)
+    SB renamable $x10, %stack.0, 0 :: (store (s8) into %stack.0)
+    SB renamable $x10, %stack.0, 4 :: (store (s8) into %stack.0 + 4)
+    renamable $x11 = SRLI renamable $x10, 24
+    SB renamable $x11, %stack.0, 3 :: (store (s8) into %stack.0 + 3)
+    renamable $x12 = SRLI renamable $x10, 16
+    SB renamable $x12, %stack.0, 2 :: (store (s8) into %stack.0 + 2)
+    renamable $x13 = SRLI renamable $x10, 8
+    SB renamable $x13, %stack.0, 1 :: (store (s8) into %stack.0 + 1)
+    SB renamable $x10, %stack.0, 8 :: (store (s8) into %stack.0 + 8)
+    SB renamable $x11, %stack.0, 7 :: (store (s8) into %stack.0 + 7)
+    SB renamable $x12, %stack.0, 6 :: (store (s8) into %stack.0 + 6)
+    SB renamable $x13, %stack.0, 5 :: (store (s8) into %stack.0 + 5)
+    SB killed renamable $x10, %stack.0, 12 :: (store (s8) into %stack.0 + 12)
+    SB renamable $x11, %stack.0, 11 :: (store (s8) into %stack.0 + 11)
+    SB renamable $x12, %stack.0, 10 :: (store (s8) into %stack.0 + 10)
+    SB renamable $x13, %stack.0, 9 :: (store (s8) into %stack.0 + 9)
+    SB killed renamable $x11, %stack.0, 15 :: (store (s8) into %stack.0 + 15)
+    SB killed renamable $x12, %stack.0, 14 :: (store (s8) into %stack.0 + 14)
+    SB killed renamable $x13, %stack.0, 13 :: (store (s8) into %stack.0 + 13)
+
+  bb.3.exit:
+    PseudoRET
+
+...