Page MenuHomePhabricator

[AVR] Improve inline assembly
Needs ReviewPublic

Authored by benshi001 on Feb 9 2021, 8:12 PM.

Details

Reviewers
dylanmckay
aykevl

Diff Detail

Unit TestsFailed

TimeTest
940 msx64 debian > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp

Event Timeline

benshi001 created this revision.Feb 9 2021, 8:12 PM
benshi001 requested review of this revision.Feb 9 2021, 8:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 8:12 PM
benshi001 added a comment.EditedFeb 9 2021, 8:24 PM

The constraints l/d/a should all accept the i16 type.

Though i32/i64 types are not supported currently, the inline asm for i8/i16 of llvm-avr
get full comtability with avr-gcc by my patch.

I will try to fix i32/i64 failure in inline-asm in the future.

aykevl added a comment.Mar 4 2021, 3:38 AM

I don't really understand what you're doing here. For example:

case 'd': // Upper registers r16..r31.
  if (VT == MVT::i8)
    return std::make_pair(0U, &AVR::LD8RegClass);
  else if (VT == MVT::i16)
    return std::make_pair(0U, &AVR::DLDREGSRegClass);
  break;

If I'm reading https://www.nongnu.org/avr-libc/user-manual/inline_asm.html correctly, this appears to imply an 8-bit register, but you're also implementing support for 16-bit registers. Why?

llvm/lib/Target/AVR/AVRISelLowering.cpp
1918–1921

Why did you remove this assert entirely? It looks like a sensible assert.

aykevl added a comment.Mar 4 2021, 3:39 AM

Oh, I missed this comment.

Though i32/i64 types are not supported currently, the inline asm for i8/i16 of llvm-avr
get full comtability with avr-gcc by my patch.

I will try to fix i32/i64 failure in inline-asm in the future.

What should i32/i64 do? AVR only has registers up to 16 bits.

benshi001 added inline comments.Mar 4 2021, 4:25 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
1918–1921

First, the assert should not be open since llvm will crash if user specified an i32 variable in inline assembly.

Second, avr-gcc does generate correct asm code for i32/i64, but llvm-avr does not. And I plan to do a further check on that.

Any way, the commented assert is unnecessary.

benshi001 marked an inline comment as done.Mar 4 2021, 4:26 AM
aykevl added a comment.Mar 4 2021, 4:28 AM

Oh, now I understand. Apparently there is a %A0/%B0 syntax that I've never seen before in any documentation with which you can select parts of a register.
Yes, this looks good to me, although perhaps @dylanmckay should also take a look?

Can you maybe add a link to this documentation somewhere?
https://www.nongnu.org/avr-libc/user-manual/inline_asm.html

llvm/lib/Target/AVR/AVRRegisterInfo.td
174

Perhaps it's a good idea to add a comment saying this is only for inline assembly?

177

It seems to me this register class is not limited to these registers: R14R13 should also be included for example.

184–186

Same here, other pairs are also possible.

I don't really understand what you're doing here. For example:

case 'd': // Upper registers r16..r31.
  if (VT == MVT::i8)
    return std::make_pair(0U, &AVR::LD8RegClass);
  else if (VT == MVT::i16)
    return std::make_pair(0U, &AVR::DLDREGSRegClass);
  break;

If I'm reading https://www.nongnu.org/avr-libc/user-manual/inline_asm.html correctly, this appears to imply an 8-bit register, but you're also implementing support for 16-bit registers. Why?

Although I expect 'd' constraint to imply an 8-bit variable, I can not prevent user specifies a 16-bit variable as d constraint on purpose. If AVR::LD8RegClass is returned for a 16-bit variable with d, llvm will crash as reported in https://bugs.llvm.org/show_bug.cgi?id=48480 .

The logic here is,

  1. return AVR::LD8RegClass for 8-bit d, which is correct and expected.
  2. return AVR::DLDREGSRegClass for 16-bit d, which is not expected, but also correct assembly.
  3. break to llvm's default process for 32/64-bit d, which is not expected, but also wrong assembly.

Actually avr-gcc accepts all 8-16-32-64-bit d and generates all correct assembly. And I just fix 8-16 bit in the patch, and left 32-64-bit in furture patches.

Oh, I missed this comment.

Though i32/i64 types are not supported currently, the inline asm for i8/i16 of llvm-avr
get full comtability with avr-gcc by my patch.

I will try to fix i32/i64 failure in inline-asm in the future.

What should i32/i64 do? AVR only has registers up to 16 bits.

My final goal is make avr-llvm generates all the same assembly as avr-gcc does. Although d implies 8-bit, avr-gcc generate all correct code for all types.

Before my patch, only 8-bit d is correct, and after my patch, 8-16-bit are all correct. And I expect to fix 32-64-bit in a future patch.

Sorry, English is not my first language, I expect I have explained everything clearly. ^_^

Also I like your tiny-go project, since I am also a go contributor. https://github.com/golang/go/commits?author=benshi001

@aykevl

aykevl added a comment.Mar 4 2021, 5:16 AM

Yes, I was not aware of the special %A0/%B0 syntax, see my comment above.

benshi001 updated this revision to Diff 328486.Mar 5 2021, 5:49 AM
benshi001 marked 2 inline comments as done.
benshi001 added inline comments.Mar 5 2021, 5:58 AM
llvm/lib/Target/AVR/AVRRegisterInfo.td
177

I am not sure R14R13 is necessary in this class (for l), since R20R19 is neither in the DLDREGS class (which corresponding to d). Although less optimized code may be generated without R14R13, it is still correct code.

I will do more careful test to see if R14R13 is needed here. Thank you.

benshi001 marked an inline comment as done.Mar 5 2021, 6:36 AM