This is an archive of the discontinued LLVM Phabricator instance.

Fix invalid Kill on callee save on RISC-V
ClosedPublic

Authored by david2050 on Oct 6 2021, 7:02 PM.

Details

Summary

A callee save may be live (specifically X1) on entry and so a spill should not mark it killed.

Diff Detail

Event Timeline

david2050 created this revision.Oct 6 2021, 7:02 PM
david2050 requested review of this revision.Oct 6 2021, 7:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 7:02 PM

An example of incorrect code in baseline is below where the spill of X1 is marked killed but is live to the following SW.

Function Live Ins: $x1

bb.0.entry:
  liveins: $x1
  $x2 = frame-setup ADDI $x2, -32
  CFI_INSTRUCTION def_cfa_offset 32
  SD killed $x1, $x2, 24
  SD killed $x8, $x2, 16
  CFI_INSTRUCTION offset $x1, -8
  CFI_INSTRUCTION offset $x8, -16
  $x8 = frame-setup ADDI $x2, 32
  CFI_INSTRUCTION def_cfa $x8, 0
  SW killed renamable $x1, $x8, -20 :: (store 4 into %ir.a)

Can you add a test case? Do you have an IR example?

jrtc27 added a comment.Oct 6 2021, 7:37 PM

I believe:

declare void @callee(i8**)

define void @caller() nounwind {
  %1 = alloca i8*
  %2 = call i8* @llvm.returnaddress(i32 0)
  store i8* %2, i8** %1
  call void @callee(i8** %1)
  ret void
}

with -stop-after=prologepilog should work. Or perhaps better to take the MIR for that with -stop-before=prologepilog and run just prologepilog on it in the test.

david2050 updated this revision to Diff 377738.Oct 6 2021, 8:36 PM

Add test case, thanks @jrtc27

david2050 updated this revision to Diff 377879.Oct 7 2021, 9:10 AM

clang format

craig.topper added inline comments.Oct 7 2021, 10:32 AM
llvm/test/CodeGen/RISCV/live_sp.ll
1 ↗(On Diff #377879)

@jrtc27 should we use update_mir_test_checks.py to get all of the CHECK lines?

2 ↗(On Diff #377879)

Drop this comment point to the review. It will be in the commit log for the file if anyone needs it.

david2050 updated this revision to Diff 377972.Oct 7 2021, 12:48 PM

Change unit test to run just prologepilog phase and use update_mir_test_checks.py

david2050 marked 2 inline comments as done.Oct 7 2021, 12:48 PM

Thanks Craig

Seems fine now. Any other comments? @craig.topper @jrtc27

llvm/test/CodeGen/RISCV/live_sp.mir
4–8 ↗(On Diff #377972)

Nit: remove this and add -march=riscv64 in RUN line instead?

asb accepted this revision.Oct 28 2021, 5:44 AM

This looks good to me., thanks you! Very minor nits on the test file:

  • I agree with Luis that modifying to use -march would be marginally better (you no longer need to worry if the datalayout string is the current standard RISC-V one for instance)
  • Although a comment linking to this review thread doesn't make sense, it would probably be helpful to add a comment to note that the live X1 register isn't incorrectly marked as killed when being spilled.
This revision is now accepted and ready to land.Oct 28 2021, 5:44 AM
This revision now requires review to proceed.Oct 28 2021, 5:44 AM
asb added a comment.Oct 28 2021, 5:46 AM

@luke957 your Herald rule seems to automatically add you as a blocking reviewer - could you please adjust it. It would probably be best to just have the rule add you as a subscriber.

luke957 resigned from this revision.Oct 28 2021, 7:33 AM

So sorry for my bad herald script.

This revision is now accepted and ready to land.Oct 28 2021, 7:33 AM
david2050 updated this revision to Diff 383166.Oct 28 2021, 2:22 PM

I made the two tweaks suggested but as it turns out I can't commit to github, can someone land this for me?

This revision was automatically updated to reflect the committed changes.
asb added a comment.Nov 2 2021, 5:03 AM

Now committed - thanks again for the patch!