Details
- Reviewers
dylanmckay aykevl - Commits
- rG86812faa5f9b: [AVR] Improve inline assembly
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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. | |
Oh, I missed this comment.
What should i32/i64 do? AVR only has registers up to 16 bits.
| 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. | |
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. | |
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,
- return AVR::LD8RegClass for 8-bit d, which is correct and expected.
- return AVR::DLDREGSRegClass for 16-bit d, which is not expected, but also correct assembly.
- 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.
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
| 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. | |
Why did you remove this assert entirely? It looks like a sensible assert.