Though the disabled test builtins.cpp can pass after my another patch https://reviews.llvm.org/D97669, it will still fail on llvm build machines, since there are no avr-libc installed on them.

Some key points,

In D97669#2688771, @Anastasia wrote:

In D97669#2687419, @benshi001 wrote:

In D97669#2685698, @Anastasia wrote:

In D97669#2678460, @benshi001 wrote:

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

No. No future pathes are needed. The avr-libc includes standard libc (headers and libs) and avr specific headers and libs. This patch fixes all of these issues. Both standard libc and avr platform specific libs can be used without any explict command line option, just like avr-gcc does.

Ok, so your patch is adding the following file

clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/stdio.h

But it doesn't seem to be used at present? If you don't need it anywhere it should not be added.

No. The stdio.h is still needed even it is not referred by any code in current patch. It works as a placeholder for the directory clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/, and this directory is needed in the test case clang/test/Driver/avr-toolchain.c of current patch, and this directory can not exist without stdio.h.

Do you know why an empty directory is not sufficient?

Overall perhaps it makes more sense if someone with more experience on AVR reviews this change.

In D97669#2685698, @Anastasia wrote:

In D97669#2678460, @benshi001 wrote:

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

No. No future pathes are needed. The avr-libc includes standard libc (headers and libs) and avr specific headers and libs. This patch fixes all of these issues. Both standard libc and avr platform specific libs can be used without any explict command line option, just like avr-gcc does.

Ok, so your patch is adding the following file

clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/stdio.h

But it doesn't seem to be used at present? If you don't need it anywhere it should not be added.

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

It seems the class ToolChain and its sub classes also have other methods not in accordance with the coding style. But fixing all of them really needs big effort. Can we fix addClangSystemIncludeArgs first, and then let https://reviews.llvm.org/D97669 pass ?

Some key points in my design,

In D96853#2661168, @Anastasia wrote:

In D96853#2660443, @benshi001 wrote:

In D96853#2659266, @Anastasia wrote:

Btw is any pointer conversion between address space 1 and the default address space allowed? I.e. is the following code valid:

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo(var1);

or

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo((int*)var1);

?

No. clang denied with error: passing 'const __attribute__((address_space(1))) int *' to parameter of type 'const int *' changes address space of pointer while avr-gcc accepted it.

It seems I need more efforts to make clang-avr fully compatible with avr-gcc.

Ok, it makes sense for clang to align with gcc in general but do you know whether or where this conversions are described? By default if not specified explicitly implicit conversions are always disallowed in ISO/IEC DTR 18037 and explicit casts are always allowed but can yeild undefined behavior if address spaces don't overlap.

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

In D96853#2659266, @Anastasia wrote:

Btw is any pointer conversion between address space 1 and the default address space allowed? I.e. is the following code valid:

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo(var1);

or

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo((int*)var1);

?

avr-gcc does

I am guessing this will only be supported by 1 target in clang? Then target address spaces make sense otherwise you can also extend the language address space enum. Are you also planning to add __flashN?
https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

fixes https://bugs.llvm.org/show_bug.cgi?id=48480

fixes https://bugs.llvm.org/show_bug.cgi?id=31568

fully fixes https://bugs.llvm.org/show_bug.cgi?id=39251
partially fixes https://bugs.llvm.org/show_bug.cgi?id=46140

This patch just fix the wrong return type in ABI. The wrong parameters passing with be fixed in another patch.

As shown in test case https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AVR/shift.ll,

Other optimization such as ashr x, 6 and ashr x, 15 can be easily added in future patches along this way.

In D96506#2585207, @aykevl wrote:

I should have explained this a while ago, sorry for neglecting this.

In D90092#2368009, @benshi001 wrote:

In D90092#2366711, @aykevl wrote:

See my comment D89047#2366709.
I think this optimization would be implemented better in SelectionDAG instead of using pseudo-instructions. That would also make it possible to optimize things like (short)x << 12.

Could you please explain more about be implemented better in SelectionDAG instead of using pseudo-instructions ? I can not figure out a way using SelectionDAG. Because I think SelectionDAG requires data flow dependency and looks like a tree structure. But The mov/clr pair seems can not be put into a tree structure.

Sorry I meant in the MachineIR phase. I think the ideal way to expand bit shift operations is as follows:

1. Lower lsl/lshr/ashr operations in SelectionDAG to pseudo instructions that have a constant immediate operand. You will only need six pseudo instructions this way: for lsl/lshr/ashr and for both 8 bit and 16 bit versions. These pseudo instructions should have the usesCustomInserter flag set.
2. Expand these instructions in AVRTargetLowering::EmitInstrWithCustomInserter. There are already a few existing expansions in there.

The big advantage is that you can do shifts before register allocation and I think it is also easier to structure the code. Also, this means you don't need as many instructions to expand in the pseudo instruction expansion pass.

Recently I have been thinking a lot about constant shifts on AVR and I've written a blog post here: https://aykevl.nl/2021/02/avr-bitshift
I think you will find it useful.

To be clear: I really appreciate your work on the AVR backend. There are many things that need to be improved, such as these bit shift operations. But I think there is a different and possibly better way to implement these expansions.

As @aykevl suggested in https://aykevl.nl/2021/02/avr-bitshift,

In D97127#2603227, @aykevl wrote:

In D97127#2602032, @benshi001 wrote:

In D97127#2600159, @aykevl wrote:

In D97127#2593329, @benshi001 wrote:

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

I think that would imply using hardcoded registers in the test. Is that what you mean?

No. I meant, you have shown a case void atomicadd(_Atomic char *val) and corresponding assembly, and I would like to see the change of the generated assembly by your patch, and wonder if the change can be added as a test case.

There are tests in llvm/test/CodeGen/AVR/atomics/load8.ll for example. However while the assembly has changed, patterns like [[RR:r[0-9]+]] are used so the test does not change. Therefore, I think the only way to show the bug is fixed is by hardcoding registers.
Or do you see another way?

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

In D96394#2602966, @aykevl wrote:

Oh, I missed this comment.

In D96394#2553152, @benshi001 wrote:

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.

In D96394#2602957, @aykevl wrote:

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?

In D97127#2600159, @aykevl wrote:

In D97127#2593329, @benshi001 wrote:

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

I think that would imply using hardcoded registers in the test. Is that what you mean?

Although the bug can be avoided via "-I /usr/lib/avr/include" in clang's command line option, I expect clang to have the save behaviour as avr-gcc.

In D97172#2600202, @aykevl wrote:

Re-upload (without changes) to try to get the buildbot working...

In D97172#2596031, @aykevl wrote:

In D97172#2593337, @benshi001 wrote:

We need to fix the build failure before commit this patch.

What do you mean? Which build failure should be fixed?

In D97131#2596214, @benshi001 wrote:

In D97131#2596186, @aykevl wrote:

In D97131#2586533, @benshi001 wrote:

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

Yes. I appreciate if you commit your patches. I will do my future work based on it (to avoid potential merge conflict between us)

In D97131#2596186, @aykevl wrote:

In D97131#2586533, @benshi001 wrote:

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

In D97669#2596073, @aykevl wrote:

Looks reasonable to me. But again, I would like this to be reviewed also by someone familiar with the internals of Clang (I'm not).

The wrong stdio.h (/usr/include/stdio.h) is included with a simple a.c like

We need to fix the build failure before commit this patch.

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

Why not commit it? I think this bug fix should be fixed ASAP.

In D96506#2585207, @aykevl wrote:

I should have explained this a while ago, sorry for neglecting this.

In D90092#2368009, @benshi001 wrote:

In D90092#2366711, @aykevl wrote:

See my comment D89047#2366709.
I think this optimization would be implemented better in SelectionDAG instead of using pseudo-instructions. That would also make it possible to optimize things like (short)x << 12.

Could you please explain more about be implemented better in SelectionDAG instead of using pseudo-instructions ? I can not figure out a way using SelectionDAG. Because I think SelectionDAG requires data flow dependency and looks like a tree structure. But The mov/clr pair seems can not be put into a tree structure.

Sorry I meant in the MachineIR phase. I think the ideal way to expand bit shift operations is as follows:

1. Lower lsl/lshr/ashr operations in SelectionDAG to pseudo instructions that have a constant immediate operand. You will only need six pseudo instructions this way: for lsl/lshr/ashr and for both 8 bit and 16 bit versions. These pseudo instructions should have the usesCustomInserter flag set.
2. Expand these instructions in AVRTargetLowering::EmitInstrWithCustomInserter. There are already a few existing expansions in there.

The big advantage is that you can do shifts before register allocation and I think it is also easier to structure the code. Also, this means you don't need as many instructions to expand in the pseudo instruction expansion pass.

Recently I have been thinking a lot about constant shifts on AVR and I've written a blog post here: https://aykevl.nl/2021/02/avr-bitshift
I think you will find it useful.

To be clear: I really appreciate your work on the AVR backend. There are many things that need to be improved, such as these bit shift operations. But I think there is a different and possibly better way to implement these expansions.

there is a lint warning.

Generally I am OK with this patch, except there is a lint warning.

In D96677#2577602, @aykevl wrote:

In D96677#2577490, @benshi001 wrote:

I am not sure such a specific pass is needed. Why __ashlsi3 is not called in other backends? Is there a config flag/option to prevent calling __ashlsi3 ?

Because most instruction sets do support 32-bit shifts but AVR does not. For example, it appears that the MSP430 has the same problem: https://reviews.llvm.org/D78663#2215170.

I've investigated whether there are any other options to this but I couldn't come up with any. The builtin calls are created inside SelectionDAG which converts non-constant shifts to library calls. Therefore, this pass converts non-constant shifts to constant shifts in a loop to match avr-gcc so that the resulting code does not contain any 32-bit non-constant shifts.

I am not sure such a specific pass is needed. Why __ashlsi3 is not called in other backends? Is there a config flag/option to prevent calling __ashlsi3 ?

In D96853#2577133, @aykevl wrote:

I am not very familiar with Clang so I can't say much about it. Although I wonder whether a macro is the right way to implement this? Is there something similar in other targets? (GPUs tend to have lots of address spaces, you could take a look there).

Also, there are lint errors, I think this patch is OK if all lint errors can be fixed.

This patch immitates avr-gcc behavior on __flash

In D96590#2561818, @aykevl wrote:

@benshi001 can you commit it or should I do it for you?

Thanks for reporting. I have submited a patch to fix that bug.
https://reviews.llvm.org/D96590