Page MenuHomePhabricator

[AVR] Remove AVRRelaxMemOperations
ClosedPublic

Authored by Patryk27 on Mar 26 2022, 12:52 PM.

Details

Summary

This commit contains a refactoring that merges AVRRelaxMemOperations
into AVRExpandPseudoInsts, so that we have a single place in code that
expands the STDWPtrQRr opcode.

Seizing the day, I've also fixed a couple of potential bugs with our
previous implementation (e.g. when the destination register was killed,
the previous implementation would try to .addDef() that killed
register, crashing LLVM in the process - that's fixed now, as proved by
the test).

In the bigger picture, this commit is the first step of fixing
https://reviews.llvm.org/D114611.

Diff Detail

Event Timeline

Patryk27 created this revision.Mar 26 2022, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2022, 12:52 PM
Patryk27 requested review of this revision.Mar 26 2022, 12:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 26 2022, 12:52 PM
Patryk27 updated this revision to Diff 418413.Mar 26 2022, 12:53 PM
Patryk27 retitled this revision from [avr] Remove AVRRelaxMemOperations to [AVR] Remove AVRRelaxMemOperations.

avr -> AVR

I have built (with -DCMAKE_BUILD_TYPE=Debug) and tested your patch, but got the following failure

FAIL: LLVM :: CodeGen/AVR/pseudo/STDWPtrQRr.mir (150 of 152)
******************** TEST 'LLVM :: CodeGen/AVR/pseudo/STDWPtrQRr.mir' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/shiben/work/llvm-project/build-x/bin/llc -O0 -run-pass=avr-expand-pseudo /home/shiben/work/llvm-project/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir -o - | /home/shiben/work/llvm-project/build-x/bin/FileCheck /home/shiben/work/llvm-project/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir
--
Exit Code: 2

Command Output (stderr):
--

# After AVR pseudo instruction expansion pass
# Machine code for function test: IsSSA, NoPHIs, TracksLiveness, NoVRegs

bb.0.entry:
  STDPtrQRr $r29r28, 3, $r0
  STDPtrQRr $r29r28, 4, $r1
  STDPtrQRr $r29r28, 3, $r0
  STDPtrQRr killed $r29r28, 4, $r1
  STDPtrQRr $r29r28, 3, killed $r0
  STDPtrQRr $r29r28, 4, killed $r1
  STDPtrQRr $r29r28, 62, $r0
  STDPtrQRr $r29r28, 63, $r1
  PUSHRr $r28, implicit-def $sp, implicit $sp
  PUSHRr $r29, implicit-def $sp, implicit $sp
  $r28 = SBCIRdK killed $r28(tied-def 0), 193, implicit-def $sreg, implicit killed $sreg
  $r29 = SBCIRdK killed $r29(tied-def 0), 255, implicit-def $sreg, implicit killed $sreg
  STPtrRr $r29r28, $r0
  STDPtrQRr $r29r28, 1, $r1
  $r29 = POPRd implicit-def $sp, implicit $sp
  $r28 = POPRd implicit-def $sp, implicit $sp
  $r28 = SBCIRdK killed $r28(tied-def 0), 193, implicit-def $sreg, implicit killed $sreg
  $r29 = SBCIRdK killed $r29(tied-def 0), 255, implicit-def $sreg, implicit killed $sreg
  STPtrRr $r29r28, $r0
  STDPtrQRr $r29r28, 1, $r1
  PUSHRr $r28, implicit-def $sp, implicit $sp
  PUSHRr $r29, implicit-def $sp, implicit $sp
  $r28 = SBCIRdK killed $r28(tied-def 0), 193, implicit-def $sreg, implicit killed $sreg
  $r29 = SBCIRdK killed $r29(tied-def 0), 255, implicit-def $sreg, implicit killed $sreg
  STPtrRr $r29r28, killed $r0
  STDPtrQRr $r29r28, 1, killed $r1
  $r29 = POPRd implicit-def $sp, implicit $sp
  $r28 = POPRd implicit-def $sp, implicit $sp

# End machine code for function test.

*** Bad machine code: Using an undefined physical register ***
- function:    test
- basic block: %bb.0 entry (0x563a083b79e8)
- instruction: $r28 = SBCIRdK killed $r28(tied-def 0), 193, implicit-def $sreg, implicit killed $sreg
- operand 4:   implicit killed $sreg
LLVM ERROR: Found 1 machine code errors.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/shiben/work/llvm-project/build-x/bin/llc -O0 -run-pass=avr-expand-pseudo /home/shiben/work/llvm-project/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir -o -
1.      Running pass 'Function Pass Manager' on module '/home/shiben/work/llvm-project/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir'.
2.      Running pass 'Verify generated machine code' on function '@test'
 #0 0x0000563a036e61a8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/shiben/work/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:0
 #1 0x0000563a036e625f PrintStackTraceSignalHandler(void*) /home/shiben/work/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:0
 #2 0x0000563a036e3e22 llvm::sys::RunSignalHandlers() /home/shiben/work/llvm-project/llvm/lib/Support/Signals.cpp:103:0
 #3 0x0000563a036e5b29 SignalHandler(int) /home/shiben/work/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:0
 #4 0x00007f1480842980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #5 0x00007f147f4f2e87 raise /build/glibc-uZu3wS/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #6 0x00007f147f4f47f1 abort /build/glibc-uZu3wS/glibc-2.27/stdlib/abort.c:81:0
 #7 0x0000563a036202ea llvm::install_bad_alloc_error_handler(void (*)(void*, char const*, bool), void*) /home/shiben/work/llvm-project/llvm/lib/Support/ErrorHandling.cpp:126:0
 #8 0x0000563a026cec3f (anonymous namespace)::MachineVerifierPass::runOnMachineFunction(llvm::MachineFunction&) /home/shiben/work/llvm-project/llvm/lib/CodeGen/MachineVerifier.cpp:307:0
 #9 0x0000563a0259c657 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/shiben/work/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:73:0
#10 0x0000563a02c7f2b3 llvm::FPPassManager::runOnFunction(llvm::Function&) /home/shiben/work/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1430:0
#11 0x0000563a02c7f633 llvm::FPPassManager::runOnModule(llvm::Module&) /home/shiben/work/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1476:0
#12 0x0000563a02c7fa7b (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/shiben/work/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1545:0
#13 0x0000563a02c7a95b llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/shiben/work/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:535:0
#14 0x0000563a02c8034f llvm::legacy::PassManager::run(llvm::Module&) /home/shiben/work/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1673:0
#15 0x0000563a018e2ad1 compileModule(char**, llvm::LLVMContext&) /home/shiben/work/llvm-project/llvm/tools/llc/llc.cpp:724:0
#16 0x0000563a018e073f main /home/shiben/work/llvm-project/llvm/tools/llc/llc.cpp:419:0
#17 0x00007f147f4d5c87 __libc_start_main /build/glibc-uZu3wS/glibc-2.27/csu/../csu/libc-start.c:344:0
#18 0x0000563a018df4ca _start (/home/shiben/work/llvm-project/build-x/bin/llc+0x14b54ca)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/shiben/work/llvm-project/build-x/bin/FileCheck /home/shiben/work/llvm-project/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir

--

********************
********************
Failed Tests (1):
  LLVM :: CodeGen/AVR/pseudo/STDWPtrQRr.mir


Testing Time: 33.51s
  Passed           : 148
  Expectedly Failed:   3
  Failed           :   1

Hmm, that's weird - I've just re-checked and everything's working correctly on my side; maybe you're testing it on an older LLVM revision? (for reference, my patch is based off of the current LLVM's main branch, which - at the time of writing this comment - is the d9cea8d3a8fff86672174780312674871729578c commit).

benshi001 added inline comments.Mar 27 2022, 5:00 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1174–1177

The above built error is caused by this line, this should be a SUBI other than a SBCI.

Hmm, that's weird - I've just re-checked and everything's working correctly on my side; maybe you're testing it on an older LLVM revision? (for reference, my patch is based off of the current LLVM's main branch, which - at the time of writing this comment - is the d9cea8d3a8fff86672174780312674871729578c commit).

I tested based on https://github.com/llvm/llvm-project/commit/674d52e8ced27bf427b3ea2c763a566ca9b8212a, which is also today's (March 27) revision. Did you build LLVM with -DCMAKE_BUILD_TYPE=Debug , it seems only debug version of llvm will get this issue be triggered.

Patryk27 added inline comments.Mar 27 2022, 5:08 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1174–1177

Out of curiosity, why SUBI? (just trying to understand what is the difference between both)

It looks like currently - https://github.com/llvm/llvm-project/blob/d9cea8d3a8fff86672174780312674871729578c/llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp#L101 - we expand to SBCI.

benshi001 added inline comments.Mar 27 2022, 5:08 AM
llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir
1

You can trigger the bug, with -verify-machineinstrs option after the -run-pass=avr-expand-pseudo option.

I think I'm using =Release instead of =Debug, that would explain the difference, yeah;

benshi001 added inline comments.Mar 27 2022, 5:11 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1174–1177

I thought this should be an old bug which has never been triggered. Actually if C bit in SREG is set before current STWPtrPdRr, then it is sure to run into wrong.

I think I'm using =Release instead of =Debug, that would explain the difference, yeah;

You need not Debug, just add -verify-machineinstrs option after the -run-pass=avr-expand-pseudo, then the bug will also be triggered.

Patryk27 updated this revision to Diff 418443.Mar 27 2022, 5:40 AM

SBCI -> SUBI

benshi001 added inline comments.Mar 27 2022, 5:40 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1174–1177

Sorry, I think this should be a SUBIW, neither SBCIW nor SUBI.

Patryk27 marked 2 inline comments as done.Mar 27 2022, 5:41 AM
Patryk27 added inline comments.
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1174–1177

Right, great catch - fixed!

Ah, sorry - I've pushed my changes at the same time you created your comment; one moment, let me change to SUBIW, then.

Ah, nevermind - looks like I've already actually used SUBIWRdK.

benshi001 added inline comments.Mar 27 2022, 5:52 AM
llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir
1

I still suggest you add a -verify-machineinstrs option after the -run-pass=avr-expand-pseudo option.

Patryk27 updated this revision to Diff 418460.Mar 27 2022, 10:54 AM

Add -verify-machineinstrs

Patryk27 added a comment.EditedMar 27 2022, 10:56 AM

Ok, I have added the switch; I think a separate patch that adds that switch to all of the AVR tests could come handy - what do you think?

benshi001 accepted this revision.Mar 27 2022, 6:01 PM

@aykevl How about your opinion? I think current form is good!

This revision is now accepted and ready to land.Mar 27 2022, 6:01 PM
benshi001 added a comment.EditedMar 27 2022, 6:03 PM

Ok, I have added the switch; I think a separate patch that adds that switch to all of the AVR tests could come handy - what do you think?

No. -verify-machineinstrs might be default in the future, which is still under discuss, since it costs longer time. Currently we just make highly risky case with explicit -verify-machineinstrs, such as yours. ^_^

Ok, I have added the switch; I think a separate patch that adds that switch to all of the AVR tests could come handy - what do you think?

No. -verify-machineinstrs might be default in the future, which is still under discuss, since it costs longer time. Currently we just make highly risky case with explicit -verify-machineinstrs, such as yours. ^_^

Okie, understood - thanks :-)

This revision was automatically updated to reflect the committed changes.