Menu
▾
▴
avr-llvm-devel — Developers mailing list.
You can subscribe to this list here.
| 2009 |
Jan
|
Feb
|
Mar
|
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
(26) |
Dec
(15) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 2010 |
Jan
|
Feb
|
Mar
|
Apr
|
May
|
Jun
(2) |
Jul
|
Aug
|
Sep
(14) |
Oct
(16) |
Nov
(36) |
Dec
(3) |
| 2011 |
Jan
|
Feb
|
Mar
(1) |
Apr
(17) |
May
(9) |
Jun
(6) |
Jul
|
Aug
|
Sep
|
Oct
(1) |
Nov
(2) |
Dec
(4) |
| 2012 |
Jan
(22) |
Feb
(12) |
Mar
(39) |
Apr
(31) |
May
(42) |
Jun
(35) |
Jul
(32) |
Aug
(2) |
Sep
(5) |
Oct
|
Nov
|
Dec
(9) |
| 2013 |
Jan
|
Feb
|
Mar
|
Apr
(2) |
May
|
Jun
(121) |
Jul
(61) |
Aug
(7) |
Sep
(8) |
Oct
(6) |
Nov
|
Dec
(1) |
| 2014 |
Jan
|
Feb
|
Mar
|
Apr
|
May
|
Jun
|
Jul
(1) |
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
| 2015 |
Jan
(1) |
Feb
|
Mar
|
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
|
From: Borja F. <bor...@gm...> - 2013-07-26 11:18:05
|
Hello Stepan, No problem with those fixes :) 2) It's interesting we get different IR, can you explain more what is going on here? This is the last bit of inline asm support, the sooner we fix it the better so we can move on to more interesting things. About compiling avrlibc: >> Also avr-clang emits wrong sequense for .S files. So I'm using CCAS=avr-gcc. What do you mean with this? What is wrong? 2) Since I compile clang as a cross compiler this is not a problem for me, all llvm binaries are prefixed with avr. >> Currently compilation stopped with unsupported movw instruction for avr2. Yes, this is going to be a problem. Try to modify the build scripts to compile everything for mmcu=avr5. We don't support any smaller devices. 2013/7/26 Stepan Dyatkovskiy <stp...@na...> > Hello Borja, > > Thanks for fixes! Especially my stupid pieces of code in AVRAsmPrinter.cpp > > > Back to your post at 01.07.2013: > > You wrote: > [quote] > Yes but the first goal now is to have inline asm feature complete xD > Then we can move into other places of the library. > > Taking a look at the previous emails this is what needs still to be done: > 1) Implement the missed optimization above for the memory constraint. > Also see if you can come with further cases. Remember to add a test case. > 2) Implement the stuff in the "C names used in assembler code" section > of the avrlibc manual. You mentioned some issues here in a previous email. > 3) Implement the a0, a1, etc... constraints. > [/quote] > > 1. Done. > 2. I checked it again. It works, though way of IR generation differs > from ARM back-end for example. > 3. Done. > > Now, I suppose, we have to work more with clang. But it is difficult > since our changes are presented as patches. > > I had tried to compile avr-libc again. I had found clang bug (generic > one: mistreating with builtins): > > http://llvm.org/bugs/show_bug.**cgi?id=16712<http://llvm.org/bugs/show_bug.cgi?id=16712> > > Also avr-clang emits wrong sequense for .S files. So I'm using > CCAS=avr-gcc. > > Steps for avr-libc compilation: > > 0. I'm working with avr-libc, svn r2403. > > 1. We also need to apply several changes for it (see the patch in > attachment). > > 2. Currently you should rename "clang" with "avr-clang". "ln -s" doesn't > works. 'avr-gcc' name was hard-coded in configure.ac, so I did the same > with 'avr-clang'. > > 3. Run configure script as below: > $AVR_LIBC_DIR/configure CC=$LLVM_BIN_DIR/avr-clang --build= --host=avr > CCAS=avr-gcc > > A have also attached my config.log file. > > Currently compilation stopped with unsupported movw instruction for avr2. > > -Stepan. > > Borja Ferrer wrote: > >> Ok, you can commit the patch now. >> >> >> 2013/7/24 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na... >> >> >> >> Borja Ferrer wrote: >> >> 1) I agree. >> >> >> 3) What happens when you use multibyte constraints with "e" or >> "b". >> >> Just had checked it. It works :-) Though I'll add test-cases for >> them too. >> >> >> 4) Hah ok, btw no need to check for i64? >> >> In one of our previous mails we decided to restrict i64 usage, since >> avr-gcc doesn't support it properly. >> >> -Stepan >> >> >> >> 2013/7/24 Stepan Dyatkovskiy <stp...@na... >> <mailto:stp...@na...> <mailto:stp...@na... >> <mailto:stp...@na...>>> >> >> Hi Borja, >> >> >> > 1) You implemented the "aN" constraint by also allowing >> b,c,d >> > constraints. I'm not sure if gcc implements this, my >> guess is >> that only >> > "a" is needed. Please check to see what gcc does here. >> >> For b,c,d avr-gcc acts like it meet %a. I think its wrong and >> untested behaviour, so I just restricted usage to single >> 'a' character. >> >> >> > 2) Make getAlternativeRegName return a char instead of a >> string >> since we >> > only need to return one letter. >> >> Done >> >> >> > 3) Related to the changes in AVRISelLowering.cpp, do we >> need to care >> > about more register constraints apart of the ones you >> already >> covered? >> >> What you mean? I think all constraints are covered now. If >> no, we >> can find out it during compilation (avr-libc, arduino, etc). >> >> >> > 4) Now that wider types have been covered, please check >> if the >> assert I >> > commented out in AVRISelLowering.cpp in the inline asm >> code can be >> > removed or what. >> >> I replaced this assertion with error message. Suppose its >> better >> than killing the compiler :-) >> >> New patch is attached. >> >> -Stepan. >> >> >> Borja Ferrer wrote: >> >> Hello Stepan, >> The patch looks good, but I have some questions/comments: >> >> 1) You implemented the "aN" constraint by also allowing >> b,c,d >> constraints. I'm not sure if gcc implements this, my >> guess is >> that only >> "a" is needed. Please check to see what gcc does here. >> 2) Make getAlternativeRegName return a char instead of >> a string >> since we >> only need to return one letter. >> 3) Related to the changes in AVRISelLowering.cpp, do we >> need to care >> about more register constraints apart of the ones you >> already >> covered? >> 4) Now that wider types have been covered, please check >> if the >> assert I >> commented out in AVRISelLowering.cpp in the inline asm >> code can be >> removed or what. >> >> >> >> 2013/7/24 Stepan Dyatkovskiy <stp...@na... >> <mailto:stp...@na...> >> <mailto:stp...@na... <mailto:stp...@na...>> >> <mailto:stp...@na... <mailto:stp...@na...> >> >> <mailto:stp...@na... <mailto:stp...@na...>>>> >> >> >> Hello Borja, >> Here is the new patch for multibyte reference. >> Please look >> at new >> tests to see what was implemented exactly. >> To fix issue I described in previous letter I've >> added new reg >> classes that contains pairs of 8 bit registers: >> If we pass i32 parameter that should be split onto >> four 8 >> bit regs, >> we set i16 register class first, then it would be >> replaced >> with 8 >> bit registers if needed. Currently I see it is the >> only >> solution >> that doesn't touch vmcore. >> >> -Stepan. >> >> Stepan Dyatkovskiy wrote: >> >> Hello Borja, >> Great. I have found out next inline-asm issue. >> When you pass i32 bit type, it could be split >> onto two >> registers >> only, >> no matter which register class you set for it. >> This >> behaviour is >> implemented in >> SelectionDAGBuilder::______**visitInlineAsm. >> >> I think I'll >> >> implement simplest version of ExpandInlineAsm >> today, >> that will fix >> operand types properly. >> >> -Stepan >> >> Borja Ferrer wrote: >> >> I've been adding fixes to pass a ton of >> regression >> in the >> Generic folder. >> Currently there are two tests that fail >> with a >> "cannot select >> build_pair" related to adding 16bit reg >> classes in the >> inline asm code. >> The rest of failing tests are related with >> not >> being able to >> run llc >> with -O0. There's a serious codegen bug >> that i spotted >> yesterday while >> doing the fixes that I need to work on. >> >> >> 2013/7/22 Borja Ferrer >> <bor...@gm... <mailto:bor...@gm...> >> <mailto:bor...@gm... >> <mailto:bor...@gm...>**> >> <mailto:bor...@gm... >> <mailto:bor...@gm...> >> <mailto:bor...@gm... >> <mailto:bor...@gm...>**>__> >> <mailto:bor...@gm... >> <mailto:bor...@gm...> >> <mailto:bor...@gm... >> <mailto:bor...@gm...>**> <mailto:bor...@gm... >> <mailto:bor...@gm...> >> <mailto:bor...@gm... >> <mailto:bor...@gm...>**>__>__>__> >> >> >> Hello Stepan, >> >> 3) Yes, it's the second case. It's >> explained in >> http://www.nongnu.org/avr-____**__libc/user-manual/inline_asm.** >> ______html<http://www.nongnu.org/avr-______libc/user-manual/inline_asm.______html> >> <http://www.nongnu.org/avr-___**_libc/user-manual/inline_asm._** >> ___html<http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html> >> > >> >> <http://www.nongnu.org/avr-___**_libc/user-manual/inline_asm._** >> ___html<http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html> >> <http://www.nongnu.org/avr-__**libc/user-manual/inline_asm.__** >> html <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html>>> >> >> >> >> >> <http://www.nongnu.org/avr-___**_libc/user-manual/inline_asm._** >> ___html<http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html> >> <http://www.nongnu.org/avr-__**libc/user-manual/inline_asm.__** >> html <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html>> >> >> <http://www.nongnu.org/avr-__**libc/user-manual/inline_asm.__** >> html <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html> >> <http://www.nongnu.org/avr-**libc/user-manual/inline_asm.**html<http://www.nongnu.org/avr-libc/user-manual/inline_asm.html> >> >>> >> but in case there are any doubts >> basically it >> works >> the same way as >> multibyte constraints except that >> when you see an >> "aN", where N is a >> number, you should print the >> alternative >> regname. Of >> course this >> should only work for X Y and Z, not >> sure how gcc >> complains when you >> use another register, or maybe it >> ignores it and >> prints the normal >> register name. >> >> >> >> >> >> >> ------------------------------**______------------------------** >> --__--__--__------------------ >> >> >> See everything from the browser to the >> database with >> AppDynamics >> Get end-to-end visibility with application >> monitoring from >> AppDynamics >> Isolate bottlenecks and diagnose root cause in >> seconds. >> Start your free trial of AppDynamics Pro today! >> http://pubads.g.doubleclick.__**____net/gampad/clk?id=** >> 48808831&__iu=____/4140/ostg.**clktrk >> >> >> <http://pubads.g.doubleclick._**___net/gampad/clk?id=48808831&** >> __iu=__/4140/ostg.clktrk >> >> <http://pubads.g.doubleclick._**_net/gampad/clk?id=48808831&** >> iu=__/4140/ostg.clktrk >> <http://pubads.g.doubleclick.**net/gampad/clk?id=48808831&iu=** >> /4140/ostg.clktrk<http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk> >> >>> >> >> ______________________________**_______________________ >> avr-llvm-devel mailing list >> avr-llvm-devel@lists.__sourcef**____orge.net<http://sourcef____orge.net> >> <http://sourcef__orge.net> >> <http://sourceforge.net> >> <mailto:avr-llvm-devel@lists. >> <mailto:avr-llvm-devel@lists.>**____sourceforge.net >> <http://sourceforge.net> >> <mailto:avr-llvm-devel@lists._**_sourceforge.net >> <mailto:avr-llvm-devel@lists.**sourceforge.net<avr...@li...> >> >>> >> https://lists.sourceforge.net/**______lists/listinfo/avr-llvm-** >> ____devel<https://lists.sourceforge.net/______lists/listinfo/avr-llvm-____devel> >> <https://lists.sourceforge.**net/____lists/listinfo/avr-** >> llvm-__devel<https://lists.sourceforge.net/____lists/listinfo/avr-llvm-__devel> >> > >> <https://lists.sourceforge.__**net/__lists/listinfo/avr-llvm-** >> __devel >> <https://lists.sourceforge.**net/__lists/listinfo/avr-llvm-** >> devel <https://lists.sourceforge.net/__lists/listinfo/avr-llvm-devel>>> >> >> >> <https://lists.sourceforge.___**_net/lists/listinfo/avr-llvm-_** >> ___devel >> >> <https://lists.sourceforge.__**net/lists/listinfo/avr-llvm-__** >> devel >> <https://lists.sourceforge.**net/lists/listinfo/avr-llvm-**devel<https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel> >> >>> >> >> >> >> >> >> >> >> > > |
|
From: Stepan D. <stp...@na...> - 2013-07-26 09:33:58
|
Hello Borja, Thanks for fixes! Especially my stupid pieces of code in AVRAsmPrinter.cpp Back to your post at 01.07.2013: You wrote: [quote] Yes but the first goal now is to have inline asm feature complete xD Then we can move into other places of the library. Taking a look at the previous emails this is what needs still to be done: 1) Implement the missed optimization above for the memory constraint. Also see if you can come with further cases. Remember to add a test case. 2) Implement the stuff in the "C names used in assembler code" section of the avrlibc manual. You mentioned some issues here in a previous email. 3) Implement the a0, a1, etc... constraints. [/quote] 1. Done. 2. I checked it again. It works, though way of IR generation differs from ARM back-end for example. 3. Done. Now, I suppose, we have to work more with clang. But it is difficult since our changes are presented as patches. I had tried to compile avr-libc again. I had found clang bug (generic one: mistreating with builtins): http://llvm.org/bugs/show_bug.cgi?id=16712 Also avr-clang emits wrong sequense for .S files. So I'm using CCAS=avr-gcc. Steps for avr-libc compilation: 0. I'm working with avr-libc, svn r2403. 1. We also need to apply several changes for it (see the patch in attachment). 2. Currently you should rename "clang" with "avr-clang". "ln -s" doesn't works. 'avr-gcc' name was hard-coded in configure.ac, so I did the same with 'avr-clang'. 3. Run configure script as below: $AVR_LIBC_DIR/configure CC=$LLVM_BIN_DIR/avr-clang --build= --host=avr CCAS=avr-gcc A have also attached my config.log file. Currently compilation stopped with unsupported movw instruction for avr2. -Stepan. Borja Ferrer wrote: > Ok, you can commit the patch now. > > > 2013/7/24 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>> > > Borja Ferrer wrote: > > 1) I agree. > > > 3) What happens when you use multibyte constraints with "e" or "b". > > Just had checked it. It works :-) Though I'll add test-cases for > them too. > > > 4) Hah ok, btw no need to check for i64? > > In one of our previous mails we decided to restrict i64 usage, since > avr-gcc doesn't support it properly. > > -Stepan > > > > 2013/7/24 Stepan Dyatkovskiy <stp...@na... > <mailto:stp...@na...> <mailto:stp...@na... > <mailto:stp...@na...>>> > > Hi Borja, > > > > 1) You implemented the "aN" constraint by also allowing > b,c,d > > constraints. I'm not sure if gcc implements this, my > guess is > that only > > "a" is needed. Please check to see what gcc does here. > > For b,c,d avr-gcc acts like it meet %a. I think its wrong and > untested behaviour, so I just restricted usage to single > 'a' character. > > > > 2) Make getAlternativeRegName return a char instead of a > string > since we > > only need to return one letter. > > Done > > > > 3) Related to the changes in AVRISelLowering.cpp, do we > need to care > > about more register constraints apart of the ones you > already > covered? > > What you mean? I think all constraints are covered now. If > no, we > can find out it during compilation (avr-libc, arduino, etc). > > > > 4) Now that wider types have been covered, please check > if the > assert I > > commented out in AVRISelLowering.cpp in the inline asm > code can be > > removed or what. > > I replaced this assertion with error message. Suppose its > better > than killing the compiler :-) > > New patch is attached. > > -Stepan. > > > Borja Ferrer wrote: > > Hello Stepan, > The patch looks good, but I have some questions/comments: > > 1) You implemented the "aN" constraint by also allowing > b,c,d > constraints. I'm not sure if gcc implements this, my > guess is > that only > "a" is needed. Please check to see what gcc does here. > 2) Make getAlternativeRegName return a char instead of > a string > since we > only need to return one letter. > 3) Related to the changes in AVRISelLowering.cpp, do we > need to care > about more register constraints apart of the ones you > already > covered? > 4) Now that wider types have been covered, please check > if the > assert I > commented out in AVRISelLowering.cpp in the inline asm > code can be > removed or what. > > > > 2013/7/24 Stepan Dyatkovskiy <stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... <mailto:stp...@na...> > > <mailto:stp...@na... <mailto:stp...@na...>>>> > > > Hello Borja, > Here is the new patch for multibyte reference. > Please look > at new > tests to see what was implemented exactly. > To fix issue I described in previous letter I've > added new reg > classes that contains pairs of 8 bit registers: > If we pass i32 parameter that should be split onto > four 8 > bit regs, > we set i16 register class first, then it would be > replaced > with 8 > bit registers if needed. Currently I see it is the > only > solution > that doesn't touch vmcore. > > -Stepan. > > Stepan Dyatkovskiy wrote: > > Hello Borja, > Great. I have found out next inline-asm issue. > When you pass i32 bit type, it could be split > onto two > registers > only, > no matter which register class you set for it. > This > behaviour is > implemented in > SelectionDAGBuilder::______visitInlineAsm. > > I think I'll > > implement simplest version of ExpandInlineAsm > today, > that will fix > operand types properly. > > -Stepan > > Borja Ferrer wrote: > > I've been adding fixes to pass a ton of > regression > in the > Generic folder. > Currently there are two tests that fail with a > "cannot select > build_pair" related to adding 16bit reg > classes in the > inline asm code. > The rest of failing tests are related with not > being able to > run llc > with -O0. There's a serious codegen bug > that i spotted > yesterday while > doing the fixes that I need to work on. > > > 2013/7/22 Borja Ferrer > <bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > <mailto:bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > <mailto:bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> <mailto:bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__>__> > > > Hello Stepan, > > 3) Yes, it's the second case. It's > explained in > http://www.nongnu.org/avr-______libc/user-manual/inline_asm.______html > <http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html> > > <http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html > <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html>> > > > > > <http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html > <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html> > > <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html > <http://www.nongnu.org/avr-libc/user-manual/inline_asm.html>>> > but in case there are any doubts > basically it > works > the same way as > multibyte constraints except that > when you see an > "aN", where N is a > number, you should print the alternative > regname. Of > course this > should only work for X Y and Z, not > sure how gcc > complains when you > use another register, or maybe it > ignores it and > prints the normal > register name. > > > > > > > ------------------------------______--------------------------__--__--__------------------ > > > See everything from the browser to the > database with > AppDynamics > Get end-to-end visibility with application > monitoring from > AppDynamics > Isolate bottlenecks and diagnose root cause in > seconds. > Start your free trial of AppDynamics Pro today! > http://pubads.g.doubleclick.______net/gampad/clk?id=48808831&__iu=____/4140/ostg.clktrk > > > <http://pubads.g.doubleclick.____net/gampad/clk?id=48808831&__iu=__/4140/ostg.clktrk > > <http://pubads.g.doubleclick.__net/gampad/clk?id=48808831&iu=__/4140/ostg.clktrk > <http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk>>> > > _____________________________________________________ > avr-llvm-devel mailing list > avr-llvm-devel@lists.__sourcef____orge.net > <http://sourcef__orge.net> > <http://sourceforge.net> > <mailto:avr-llvm-devel@lists. > <mailto:avr-llvm-devel@lists.>____sourceforge.net > <http://sourceforge.net> > <mailto:avr-llvm-devel@lists.__sourceforge.net > <mailto:avr...@li...>>> > https://lists.sourceforge.net/______lists/listinfo/avr-llvm-____devel > <https://lists.sourceforge.net/____lists/listinfo/avr-llvm-__devel> > <https://lists.sourceforge.__net/__lists/listinfo/avr-llvm-__devel > <https://lists.sourceforge.net/__lists/listinfo/avr-llvm-devel>> > > > <https://lists.sourceforge.____net/lists/listinfo/avr-llvm-____devel > > <https://lists.sourceforge.__net/lists/listinfo/avr-llvm-__devel > <https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel>>> > > > > > > > |
|
From: Borja F. <bor...@gm...> - 2013-07-24 16:56:14
|
Ok, you can commit the patch now. 2013/7/24 Stepan Dyatkovskiy <stp...@na...> > Borja Ferrer wrote: > >> 1) I agree. >> > > 3) What happens when you use multibyte constraints with "e" or "b". >> > Just had checked it. It works :-) Though I'll add test-cases for them too. > > > 4) Hah ok, btw no need to check for i64? >> > In one of our previous mails we decided to restrict i64 usage, since > avr-gcc doesn't support it properly. > > -Stepan > > >> >> 2013/7/24 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na... >> >> >> >> Hi Borja, >> >> >> > 1) You implemented the "aN" constraint by also allowing b,c,d >> > constraints. I'm not sure if gcc implements this, my guess is >> that only >> > "a" is needed. Please check to see what gcc does here. >> >> For b,c,d avr-gcc acts like it meet %a. I think its wrong and >> untested behaviour, so I just restricted usage to single 'a' >> character. >> >> >> > 2) Make getAlternativeRegName return a char instead of a string >> since we >> > only need to return one letter. >> >> Done >> >> >> > 3) Related to the changes in AVRISelLowering.cpp, do we need to >> care >> > about more register constraints apart of the ones you already >> covered? >> >> What you mean? I think all constraints are covered now. If no, we >> can find out it during compilation (avr-libc, arduino, etc). >> >> >> > 4) Now that wider types have been covered, please check if the >> assert I >> > commented out in AVRISelLowering.cpp in the inline asm code can be >> > removed or what. >> >> I replaced this assertion with error message. Suppose its better >> than killing the compiler :-) >> >> New patch is attached. >> >> -Stepan. >> >> >> Borja Ferrer wrote: >> >> Hello Stepan, >> The patch looks good, but I have some questions/comments: >> >> 1) You implemented the "aN" constraint by also allowing b,c,d >> constraints. I'm not sure if gcc implements this, my guess is >> that only >> "a" is needed. Please check to see what gcc does here. >> 2) Make getAlternativeRegName return a char instead of a string >> since we >> only need to return one letter. >> 3) Related to the changes in AVRISelLowering.cpp, do we need to >> care >> about more register constraints apart of the ones you already >> covered? >> 4) Now that wider types have been covered, please check if the >> assert I >> commented out in AVRISelLowering.cpp in the inline asm code can be >> removed or what. >> >> >> >> 2013/7/24 Stepan Dyatkovskiy <stp...@na... >> <mailto:stp...@na...> <mailto:stp...@na... >> >> <mailto:stp...@na...>>> >> >> >> Hello Borja, >> Here is the new patch for multibyte reference. Please look >> at new >> tests to see what was implemented exactly. >> To fix issue I described in previous letter I've added new >> reg >> classes that contains pairs of 8 bit registers: >> If we pass i32 parameter that should be split onto four 8 >> bit regs, >> we set i16 register class first, then it would be replaced >> with 8 >> bit registers if needed. Currently I see it is the only >> solution >> that doesn't touch vmcore. >> >> -Stepan. >> >> Stepan Dyatkovskiy wrote: >> >> Hello Borja, >> Great. I have found out next inline-asm issue. >> When you pass i32 bit type, it could be split onto two >> registers >> only, >> no matter which register class you set for it. This >> behaviour is >> implemented in SelectionDAGBuilder::____** >> visitInlineAsm. >> >> I think I'll >> >> implement simplest version of ExpandInlineAsm today, >> that will fix >> operand types properly. >> >> -Stepan >> >> Borja Ferrer wrote: >> >> I've been adding fixes to pass a ton of regression >> in the >> Generic folder. >> Currently there are two tests that fail with a >> "cannot select >> build_pair" related to adding 16bit reg classes in >> the >> inline asm code. >> The rest of failing tests are related with not >> being able to >> run llc >> with -O0. There's a serious codegen bug that i >> spotted >> yesterday while >> doing the fixes that I need to work on. >> >> >> 2013/7/22 Borja Ferrer <bor...@gm... >> <mailto:bor...@gm...> >> <mailto:bor...@gm... >> <mailto:bor...@gm...>**> >> <mailto:bor...@gm... >> <mailto:bor...@gm...> <mailto:bor...@gm... >> <mailto:bor...@gm...>**>__>__> >> >> >> Hello Stepan, >> >> 3) Yes, it's the second case. It's explained in >> http://www.nongnu.org/avr-____**libc/user-manual/inline_asm.__** >> __html<http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html> >> <http://www.nongnu.org/avr-__**libc/user-manual/inline_asm.__** >> html <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html>> >> >> >> >> <http://www.nongnu.org/avr-__**libc/user-manual/inline_asm.__** >> html <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html> >> <http://www.nongnu.org/avr-**libc/user-manual/inline_asm.**html<http://www.nongnu.org/avr-libc/user-manual/inline_asm.html> >> >> >> but in case there are any doubts basically it >> works >> the same way as >> multibyte constraints except that when you see >> an >> "aN", where N is a >> number, you should print the alternative >> regname. Of >> course this >> should only work for X Y and Z, not sure how >> gcc >> complains when you >> use another register, or maybe it ignores it >> and >> prints the normal >> register name. >> >> >> >> >> >> ------------------------------**____--------------------------** >> --__--__------------------ >> >> >> See everything from the browser to the database with >> AppDynamics >> Get end-to-end visibility with application monitoring >> from >> AppDynamics >> Isolate bottlenecks and diagnose root cause in seconds. >> Start your free trial of AppDynamics Pro today! >> http://pubads.g.doubleclick.__**__net/gampad/clk?id=48808831&** >> iu=____/4140/ostg.clktrk >> >> <http://pubads.g.doubleclick._**_net/gampad/clk?id=48808831&** >> iu=__/4140/ostg.clktrk >> <http://pubads.g.doubleclick.**net/gampad/clk?id=48808831&iu=** >> /4140/ostg.clktrk<http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk> >> >> >> ______________________________**_____________________ >> avr-llvm-devel mailing list >> avr-llvm-devel@lists.__sourcef**__orge.net<http://sourcef__orge.net> >> <http://sourceforge.net> >> <mailto:avr-llvm-devel@lists._**_sourceforge.net >> <mailto:avr-llvm-devel@lists.**sourceforge.net<avr...@li...> >> >> >> https://lists.sourceforge.net/**____lists/listinfo/avr-llvm-__** >> devel <https://lists.sourceforge.net/____lists/listinfo/avr-llvm-__devel>< >> https://lists.sourceforge.**net/__lists/listinfo/avr-llvm-**devel<https://lists.sourceforge.net/__lists/listinfo/avr-llvm-devel> >> > >> >> <https://lists.sourceforge.__**net/lists/listinfo/avr-llvm-__** >> devel >> <https://lists.sourceforge.**net/lists/listinfo/avr-llvm-**devel<https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel> >> >> >> >> >> >> >> >> > |
|
From: Stepan D. <stp...@na...> - 2013-07-24 15:58:57
|
Borja Ferrer wrote: > 1) I agree. > 3) What happens when you use multibyte constraints with "e" or "b". Just had checked it. It works :-) Though I'll add test-cases for them too. > 4) Hah ok, btw no need to check for i64? In one of our previous mails we decided to restrict i64 usage, since avr-gcc doesn't support it properly. -Stepan > > > 2013/7/24 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>> > > Hi Borja, > > > > 1) You implemented the "aN" constraint by also allowing b,c,d > > constraints. I'm not sure if gcc implements this, my guess is > that only > > "a" is needed. Please check to see what gcc does here. > > For b,c,d avr-gcc acts like it meet %a. I think its wrong and > untested behaviour, so I just restricted usage to single 'a' character. > > > > 2) Make getAlternativeRegName return a char instead of a string > since we > > only need to return one letter. > > Done > > > > 3) Related to the changes in AVRISelLowering.cpp, do we need to care > > about more register constraints apart of the ones you already > covered? > > What you mean? I think all constraints are covered now. If no, we > can find out it during compilation (avr-libc, arduino, etc). > > > > 4) Now that wider types have been covered, please check if the > assert I > > commented out in AVRISelLowering.cpp in the inline asm code can be > > removed or what. > > I replaced this assertion with error message. Suppose its better > than killing the compiler :-) > > New patch is attached. > > -Stepan. > > > Borja Ferrer wrote: > > Hello Stepan, > The patch looks good, but I have some questions/comments: > > 1) You implemented the "aN" constraint by also allowing b,c,d > constraints. I'm not sure if gcc implements this, my guess is > that only > "a" is needed. Please check to see what gcc does here. > 2) Make getAlternativeRegName return a char instead of a string > since we > only need to return one letter. > 3) Related to the changes in AVRISelLowering.cpp, do we need to care > about more register constraints apart of the ones you already > covered? > 4) Now that wider types have been covered, please check if the > assert I > commented out in AVRISelLowering.cpp in the inline asm code can be > removed or what. > > > > 2013/7/24 Stepan Dyatkovskiy <stp...@na... > <mailto:stp...@na...> <mailto:stp...@na... > <mailto:stp...@na...>>> > > > Hello Borja, > Here is the new patch for multibyte reference. Please look > at new > tests to see what was implemented exactly. > To fix issue I described in previous letter I've added new reg > classes that contains pairs of 8 bit registers: > If we pass i32 parameter that should be split onto four 8 > bit regs, > we set i16 register class first, then it would be replaced > with 8 > bit registers if needed. Currently I see it is the only > solution > that doesn't touch vmcore. > > -Stepan. > > Stepan Dyatkovskiy wrote: > > Hello Borja, > Great. I have found out next inline-asm issue. > When you pass i32 bit type, it could be split onto two > registers > only, > no matter which register class you set for it. This > behaviour is > implemented in SelectionDAGBuilder::____visitInlineAsm. > I think I'll > > implement simplest version of ExpandInlineAsm today, > that will fix > operand types properly. > > -Stepan > > Borja Ferrer wrote: > > I've been adding fixes to pass a ton of regression > in the > Generic folder. > Currently there are two tests that fail with a > "cannot select > build_pair" related to adding 16bit reg classes in the > inline asm code. > The rest of failing tests are related with not > being able to > run llc > with -O0. There's a serious codegen bug that i spotted > yesterday while > doing the fixes that I need to work on. > > > 2013/7/22 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > <mailto:bor...@gm... > <mailto:bor...@gm...> <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > Hello Stepan, > > 3) Yes, it's the second case. It's explained in > http://www.nongnu.org/avr-____libc/user-manual/inline_asm.____html > <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html> > > > <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html > <http://www.nongnu.org/avr-libc/user-manual/inline_asm.html>> > but in case there are any doubts basically it > works > the same way as > multibyte constraints except that when you see an > "aN", where N is a > number, you should print the alternative > regname. Of > course this > should only work for X Y and Z, not sure how gcc > complains when you > use another register, or maybe it ignores it and > prints the normal > register name. > > > > > > ------------------------------____----------------------------__--__------------------ > > See everything from the browser to the database with > AppDynamics > Get end-to-end visibility with application monitoring from > AppDynamics > Isolate bottlenecks and diagnose root cause in seconds. > Start your free trial of AppDynamics Pro today! > http://pubads.g.doubleclick.____net/gampad/clk?id=48808831&iu=____/4140/ostg.clktrk > > <http://pubads.g.doubleclick.__net/gampad/clk?id=48808831&iu=__/4140/ostg.clktrk > <http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk>> > ___________________________________________________ > avr-llvm-devel mailing list > avr-llvm-devel@lists.__sourcef__orge.net > <http://sourceforge.net> > <mailto:avr-llvm-devel@lists.__sourceforge.net > <mailto:avr...@li...>> > https://lists.sourceforge.net/____lists/listinfo/avr-llvm-__devel <https://lists.sourceforge.net/__lists/listinfo/avr-llvm-devel> > > <https://lists.sourceforge.__net/lists/listinfo/avr-llvm-__devel > <https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel>> > > > > > |
|
From: Borja F. <bor...@gm...> - 2013-07-24 15:10:40
|
1) I agree. 3) What happens when you use multibyte constraints with "e" or "b". 4) Hah ok, btw no need to check for i64? 2013/7/24 Stepan Dyatkovskiy <stp...@na...> > Hi Borja, > > > > 1) You implemented the "aN" constraint by also allowing b,c,d > > constraints. I'm not sure if gcc implements this, my guess is that only > > "a" is needed. Please check to see what gcc does here. > > For b,c,d avr-gcc acts like it meet %a. I think its wrong and untested > behaviour, so I just restricted usage to single 'a' character. > > > > 2) Make getAlternativeRegName return a char instead of a string since we > > only need to return one letter. > > Done > > > > 3) Related to the changes in AVRISelLowering.cpp, do we need to care > > about more register constraints apart of the ones you already covered? > > What you mean? I think all constraints are covered now. If no, we can find > out it during compilation (avr-libc, arduino, etc). > > > > 4) Now that wider types have been covered, please check if the assert I > > commented out in AVRISelLowering.cpp in the inline asm code can be > > removed or what. > > I replaced this assertion with error message. Suppose its better than > killing the compiler :-) > > New patch is attached. > > -Stepan. > > > Borja Ferrer wrote: > >> Hello Stepan, >> The patch looks good, but I have some questions/comments: >> >> 1) You implemented the "aN" constraint by also allowing b,c,d >> constraints. I'm not sure if gcc implements this, my guess is that only >> "a" is needed. Please check to see what gcc does here. >> 2) Make getAlternativeRegName return a char instead of a string since we >> only need to return one letter. >> 3) Related to the changes in AVRISelLowering.cpp, do we need to care >> about more register constraints apart of the ones you already covered? >> 4) Now that wider types have been covered, please check if the assert I >> commented out in AVRISelLowering.cpp in the inline asm code can be >> removed or what. >> >> >> >> 2013/7/24 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na... >> >> >> >> >> Hello Borja, >> Here is the new patch for multibyte reference. Please look at new >> tests to see what was implemented exactly. >> To fix issue I described in previous letter I've added new reg >> classes that contains pairs of 8 bit registers: >> If we pass i32 parameter that should be split onto four 8 bit regs, >> we set i16 register class first, then it would be replaced with 8 >> bit registers if needed. Currently I see it is the only solution >> that doesn't touch vmcore. >> >> -Stepan. >> >> Stepan Dyatkovskiy wrote: >> >> Hello Borja, >> Great. I have found out next inline-asm issue. >> When you pass i32 bit type, it could be split onto two registers >> only, >> no matter which register class you set for it. This behaviour is >> implemented in SelectionDAGBuilder::__**visitInlineAsm. I think >> I'll >> >> implement simplest version of ExpandInlineAsm today, that will fix >> operand types properly. >> >> -Stepan >> >> Borja Ferrer wrote: >> >> I've been adding fixes to pass a ton of regression in the >> Generic folder. >> Currently there are two tests that fail with a "cannot select >> build_pair" related to adding 16bit reg classes in the >> inline asm code. >> The rest of failing tests are related with not being able to >> run llc >> with -O0. There's a serious codegen bug that i spotted >> yesterday while >> doing the fixes that I need to work on. >> >> >> 2013/7/22 Borja Ferrer <bor...@gm... >> <mailto:bor...@gm...> >> <mailto:bor...@gm... <mailto:bor...@gm...>* >> *>__> >> >> Hello Stepan, >> >> 3) Yes, it's the second case. It's explained in >> http://www.nongnu.org/avr-__**libc/user-manual/inline_asm.__* >> *html <http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html> >> >> <http://www.nongnu.org/avr-**libc/user-manual/inline_asm.** >> html <http://www.nongnu.org/avr-libc/user-manual/inline_asm.html>> >> but in case there are any doubts basically it works >> the same way as >> multibyte constraints except that when you see an >> "aN", where N is a >> number, you should print the alternative regname. Of >> course this >> should only work for X Y and Z, not sure how gcc >> complains when you >> use another register, or maybe it ignores it and >> prints the normal >> register name. >> >> >> >> >> ------------------------------**__----------------------------** >> --__------------------ >> >> See everything from the browser to the database with AppDynamics >> Get end-to-end visibility with application monitoring from >> AppDynamics >> Isolate bottlenecks and diagnose root cause in seconds. >> Start your free trial of AppDynamics Pro today! >> http://pubads.g.doubleclick.__**net/gampad/clk?id=48808831&iu=** >> __/4140/ostg.clktrk >> <http://pubads.g.doubleclick.**net/gampad/clk?id=48808831&iu=** >> /4140/ostg.clktrk<http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk> >> > >> ______________________________**___________________ >> avr-llvm-devel mailing list >> avr-llvm-devel@lists.__sourcef**orge.net <http://sourceforge.net> >> <mailto:avr-llvm-devel@lists.**sourceforge.net<avr...@li...> >> > >> https://lists.sourceforge.net/**__lists/listinfo/avr-llvm-**devel<https://lists.sourceforge.net/__lists/listinfo/avr-llvm-devel> >> <https://lists.sourceforge.**net/lists/listinfo/avr-llvm-**devel<https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel> >> > >> >> >> >> > |
|
From: Stepan D. <stp...@na...> - 2013-07-24 15:00:18
|
Hi Borja, > 1) You implemented the "aN" constraint by also allowing b,c,d > constraints. I'm not sure if gcc implements this, my guess is that only > "a" is needed. Please check to see what gcc does here. For b,c,d avr-gcc acts like it meet %a. I think its wrong and untested behaviour, so I just restricted usage to single 'a' character. > 2) Make getAlternativeRegName return a char instead of a string since we > only need to return one letter. Done > 3) Related to the changes in AVRISelLowering.cpp, do we need to care > about more register constraints apart of the ones you already covered? What you mean? I think all constraints are covered now. If no, we can find out it during compilation (avr-libc, arduino, etc). > 4) Now that wider types have been covered, please check if the assert I > commented out in AVRISelLowering.cpp in the inline asm code can be > removed or what. I replaced this assertion with error message. Suppose its better than killing the compiler :-) New patch is attached. -Stepan. Borja Ferrer wrote: > Hello Stepan, > The patch looks good, but I have some questions/comments: > > 1) You implemented the "aN" constraint by also allowing b,c,d > constraints. I'm not sure if gcc implements this, my guess is that only > "a" is needed. Please check to see what gcc does here. > 2) Make getAlternativeRegName return a char instead of a string since we > only need to return one letter. > 3) Related to the changes in AVRISelLowering.cpp, do we need to care > about more register constraints apart of the ones you already covered? > 4) Now that wider types have been covered, please check if the assert I > commented out in AVRISelLowering.cpp in the inline asm code can be > removed or what. > > > > 2013/7/24 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>> > > Hello Borja, > Here is the new patch for multibyte reference. Please look at new > tests to see what was implemented exactly. > To fix issue I described in previous letter I've added new reg > classes that contains pairs of 8 bit registers: > If we pass i32 parameter that should be split onto four 8 bit regs, > we set i16 register class first, then it would be replaced with 8 > bit registers if needed. Currently I see it is the only solution > that doesn't touch vmcore. > > -Stepan. > > Stepan Dyatkovskiy wrote: > > Hello Borja, > Great. I have found out next inline-asm issue. > When you pass i32 bit type, it could be split onto two registers > only, > no matter which register class you set for it. This behaviour is > implemented in SelectionDAGBuilder::__visitInlineAsm. I think I'll > implement simplest version of ExpandInlineAsm today, that will fix > operand types properly. > > -Stepan > > Borja Ferrer wrote: > > I've been adding fixes to pass a ton of regression in the > Generic folder. > Currently there are two tests that fail with a "cannot select > build_pair" related to adding 16bit reg classes in the > inline asm code. > The rest of failing tests are related with not being able to > run llc > with -O0. There's a serious codegen bug that i spotted > yesterday while > doing the fixes that I need to work on. > > > 2013/7/22 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... <mailto:bor...@gm...>>__> > > Hello Stepan, > > 3) Yes, it's the second case. It's explained in > http://www.nongnu.org/avr-__libc/user-manual/inline_asm.__html > <http://www.nongnu.org/avr-libc/user-manual/inline_asm.html> > but in case there are any doubts basically it works > the same way as > multibyte constraints except that when you see an > "aN", where N is a > number, you should print the alternative regname. Of > course this > should only work for X Y and Z, not sure how gcc > complains when you > use another register, or maybe it ignores it and > prints the normal > register name. > > > > > ------------------------------__------------------------------__------------------ > See everything from the browser to the database with AppDynamics > Get end-to-end visibility with application monitoring from > AppDynamics > Isolate bottlenecks and diagnose root cause in seconds. > Start your free trial of AppDynamics Pro today! > http://pubads.g.doubleclick.__net/gampad/clk?id=48808831&iu=__/4140/ostg.clktrk > <http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk> > _________________________________________________ > avr-llvm-devel mailing list > avr-llvm-devel@lists.__sourceforge.net > <mailto:avr...@li...> > https://lists.sourceforge.net/__lists/listinfo/avr-llvm-devel > <https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel> > > > |
|
From: Borja F. <bor...@gm...> - 2013-07-24 14:28:05
|
Hello Stepan, The patch looks good, but I have some questions/comments: 1) You implemented the "aN" constraint by also allowing b,c,d constraints. I'm not sure if gcc implements this, my guess is that only "a" is needed. Please check to see what gcc does here. 2) Make getAlternativeRegName return a char instead of a string since we only need to return one letter. 3) Related to the changes in AVRISelLowering.cpp, do we need to care about more register constraints apart of the ones you already covered? 4) Now that wider types have been covered, please check if the assert I commented out in AVRISelLowering.cpp in the inline asm code can be removed or what. 2013/7/24 Stepan Dyatkovskiy <stp...@na...> > Hello Borja, > Here is the new patch for multibyte reference. Please look at new tests to > see what was implemented exactly. > To fix issue I described in previous letter I've added new reg classes > that contains pairs of 8 bit registers: > If we pass i32 parameter that should be split onto four 8 bit regs, we set > i16 register class first, then it would be replaced with 8 bit registers if > needed. Currently I see it is the only solution that doesn't touch vmcore. > > -Stepan. > > Stepan Dyatkovskiy wrote: > >> Hello Borja, >> Great. I have found out next inline-asm issue. >> When you pass i32 bit type, it could be split onto two registers only, >> no matter which register class you set for it. This behaviour is >> implemented in SelectionDAGBuilder::**visitInlineAsm. I think I'll >> implement simplest version of ExpandInlineAsm today, that will fix >> operand types properly. >> >> -Stepan >> >> Borja Ferrer wrote: >> >>> I've been adding fixes to pass a ton of regression in the Generic folder. >>> Currently there are two tests that fail with a "cannot select >>> build_pair" related to adding 16bit reg classes in the inline asm code. >>> The rest of failing tests are related with not being able to run llc >>> with -O0. There's a serious codegen bug that i spotted yesterday while >>> doing the fixes that I need to work on. >>> >>> >>> 2013/7/22 Borja Ferrer <bor...@gm... >>> <mailto:bor...@gm...>**> >>> >>> Hello Stepan, >>> >>> 3) Yes, it's the second case. It's explained in >>> http://www.nongnu.org/avr-**libc/user-manual/inline_asm.**html<http://www.nongnu.org/avr-libc/user-manual/inline_asm.html> >>> but in case there are any doubts basically it works the same way as >>> multibyte constraints except that when you see an "aN", where N is a >>> number, you should print the alternative regname. Of course this >>> should only work for X Y and Z, not sure how gcc complains when you >>> use another register, or maybe it ignores it and prints the normal >>> register name. >>> >>> >>> >> >> ------------------------------**------------------------------** >> ------------------ >> See everything from the browser to the database with AppDynamics >> Get end-to-end visibility with application monitoring from AppDynamics >> Isolate bottlenecks and diagnose root cause in seconds. >> Start your free trial of AppDynamics Pro today! >> http://pubads.g.doubleclick.**net/gampad/clk?id=48808831&iu=** >> /4140/ostg.clktrk<http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk> >> ______________________________**_________________ >> avr-llvm-devel mailing list >> avr-llvm-devel@lists.**sourceforge.net<avr...@li...> >> https://lists.sourceforge.net/**lists/listinfo/avr-llvm-devel<https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel> >> >> > |
|
From: Stepan D. <stp...@na...> - 2013-07-24 13:09:02
|
Hello Borja, Here is the new patch for multibyte reference. Please look at new tests to see what was implemented exactly. To fix issue I described in previous letter I've added new reg classes that contains pairs of 8 bit registers: If we pass i32 parameter that should be split onto four 8 bit regs, we set i16 register class first, then it would be replaced with 8 bit registers if needed. Currently I see it is the only solution that doesn't touch vmcore. -Stepan. Stepan Dyatkovskiy wrote: > Hello Borja, > Great. I have found out next inline-asm issue. > When you pass i32 bit type, it could be split onto two registers only, > no matter which register class you set for it. This behaviour is > implemented in SelectionDAGBuilder::visitInlineAsm. I think I'll > implement simplest version of ExpandInlineAsm today, that will fix > operand types properly. > > -Stepan > > Borja Ferrer wrote: >> I've been adding fixes to pass a ton of regression in the Generic folder. >> Currently there are two tests that fail with a "cannot select >> build_pair" related to adding 16bit reg classes in the inline asm code. >> The rest of failing tests are related with not being able to run llc >> with -O0. There's a serious codegen bug that i spotted yesterday while >> doing the fixes that I need to work on. >> >> >> 2013/7/22 Borja Ferrer <bor...@gm... >> <mailto:bor...@gm...>> >> >> Hello Stepan, >> >> 3) Yes, it's the second case. It's explained in >> http://www.nongnu.org/avr-libc/user-manual/inline_asm.html >> but in case there are any doubts basically it works the same way as >> multibyte constraints except that when you see an "aN", where N is a >> number, you should print the alternative regname. Of course this >> should only work for X Y and Z, not sure how gcc complains when you >> use another register, or maybe it ignores it and prints the normal >> register name. >> >> > > > ------------------------------------------------------------------------------ > See everything from the browser to the database with AppDynamics > Get end-to-end visibility with application monitoring from AppDynamics > Isolate bottlenecks and diagnose root cause in seconds. > Start your free trial of AppDynamics Pro today! > http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk > _______________________________________________ > avr-llvm-devel mailing list > avr...@li... > https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel > |
|
From: Stepan D. <stp...@na...> - 2013-07-24 07:21:27
|
Hello Borja, Great. I have found out next inline-asm issue. When you pass i32 bit type, it could be split onto two registers only, no matter which register class you set for it. This behaviour is implemented in SelectionDAGBuilder::visitInlineAsm. I think I'll implement simplest version of ExpandInlineAsm today, that will fix operand types properly. -Stepan Borja Ferrer wrote: > I've been adding fixes to pass a ton of regression in the Generic folder. > Currently there are two tests that fail with a "cannot select > build_pair" related to adding 16bit reg classes in the inline asm code. > The rest of failing tests are related with not being able to run llc > with -O0. There's a serious codegen bug that i spotted yesterday while > doing the fixes that I need to work on. > > > 2013/7/22 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...>> > > Hello Stepan, > > 3) Yes, it's the second case. It's explained in > http://www.nongnu.org/avr-libc/user-manual/inline_asm.html > but in case there are any doubts basically it works the same way as > multibyte constraints except that when you see an "aN", where N is a > number, you should print the alternative regname. Of course this > should only work for X Y and Z, not sure how gcc complains when you > use another register, or maybe it ignores it and prints the normal > register name. > > |
|
From: Borja F. <bor...@gm...> - 2013-07-23 19:04:38
|
I've been adding fixes to pass a ton of regression in the Generic folder. Currently there are two tests that fail with a "cannot select build_pair" related to adding 16bit reg classes in the inline asm code. The rest of failing tests are related with not being able to run llc with -O0. There's a serious codegen bug that i spotted yesterday while doing the fixes that I need to work on. 2013/7/22 Borja Ferrer <bor...@gm...> > Hello Stepan, > > 3) Yes, it's the second case. It's explained in > http://www.nongnu.org/avr-libc/user-manual/inline_asm.html > but in case there are any doubts basically it works the same way as > multibyte constraints except that when you see an "aN", where N is a > number, you should print the alternative regname. Of course this should > only work for X Y and Z, not sure how gcc complains when you use another > register, or maybe it ignores it and prints the normal register name. > > |
|
From: Borja F. <bor...@gm...> - 2013-07-22 18:16:21
|
Hello Stepan, 3) Yes, it's the second case. It's explained in http://www.nongnu.org/avr-libc/user-manual/inline_asm.html but in case there are any doubts basically it works the same way as multibyte constraints except that when you see an "aN", where N is a number, you should print the alternative regname. Of course this should only work for X Y and Z, not sure how gcc complains when you use another register, or maybe it ignores it and prints the normal register name. |
|
From: Stepan D. <stp...@na...> - 2013-07-22 17:19:29
|
Hi Borja,
> 3) Implement the a0, a1, etc... constraints.
Not sure what you mean here? a0 as first register from "a" constraint:
asm("instr %0": "a0"(var):);
Or %a0 like that:
asm volatile(
"cli"
"ld r24, %a0"
"inc r24"
"st %a0, r24"
"sei"
:
: "e" (ptr)
: "r24"
);
-Stepan
Borja Ferrer wrote:
> Yes but the first goal now is to have inline asm feature complete xD
> Then we can move into other places of the library.
>
> Taking a look at the previous emails this is what needs still to be done:
> 1) Implement the missed optimization above for the memory constraint.
> Also see if you can come with further cases. Remember to add a test case.
> 2) Implement the stuff in the "C names used in assembler code" section
> of the avrlibc manual. You mentioned some issues here in a previous email.
> 3) Implement the a0, a1, etc... constraints.
>
>
>
> 2013/7/1 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>>
>
> Hi Borja.
> 1. OK. Will add it.
> 2.
>
> > movw r30, r24
> > adiw r30, 1
> Yup, we can replace it with "load r24, Z+1". I'll improve
> LowerINLINEASM a bit then.
>
> 3. If I'm got right, our main goal now is to get avr-libc
> compilable? Whould you tell me would else remained to do with
> inline-asm?
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> 1) Yes, an assert could do it. Fixup your patch to only allow A
> down to
> D constraints.
> 2) This code was compiled with -O3, cant you reproduce it?
>
>
> 2013/6/29 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm... <mailto:bor...@gm...>>__>
>
>
> Ok more things, I've looked into the 64bit constraints and gcc
> doesnt seem to support them, for this code:
>
> unsigned long long delay(unsigned long long a, unsigned
> long long b)
>
> {
> uint8_t cnt;
> asm volatile (
> "add %A0, %A1" "\n\t"
> "add %B0, %B1" "\n\t"
> "add %C0, %C1" "\n\t"
> "add %D0, %D1" "\n\t"
> "add %E0, %E1" "\n\t"
> "add %F0, %F1" "\n\t"
> "add %G0, %G1" "\n\t"
> "add %H0, %H1" "\n\t"
> : "=r" (a)
> : "r" (b));
> return a;
> }
>
> gcc produces (ignoring frames):
> movw r18,r10
> movw r20,r12
> movw r22,r14
> movw r24,r16
> /* #APP */
> ; 266 "test.c" 1
> add r10, r18
> add r11, r19
> add r12, r20
> add r13, r21
> add r10, r18
> add r10, r18
> add r10, r18
> add r10, r18
>
> ; 0 "" 2
> /* #NOAPP */
> movw r18,r10
> movw r20,r12
> movw r22,r14
> movw r24,r16
>
> notice how the last 4 add instructions are wrong.
>
>
>
> 2013/6/29 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
>
> Ignore my previous email, it was my fault. Now, for the
> following code:
>
> uint8_t delay(unsigned char *p)
>
> {
> uint8_t cnt=8;
> asm volatile (
> "ld %0, %1" "\n\t"
> : "=r" (cnt)
> : "Q" (p[1]));
> return cnt;
> }
>
> we get:
> movw r30, r24
> adiw r30, 1
> ;APP
> ld r24, Z
>
> ;NO_APP
> ret
>
> Ideally, that adiw should be folded into the load.
>
>
>
> 2013/6/29 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
>
> Stepan I've been testing the memory constraint but im
> getting an assertion for the following code:
>
> uint8_t delay(unsigned char **p)
> {
> uint8_t cnt=8;
> asm volatile (
> "ld %0, %1" "\n\t"
> : "=r" (cnt)
> : "Q" (p[7]));
> return cnt;
> }
>
> What's wrong in here?
>
>
> 2013/6/29 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
>
> 1) Ok then, I will look into it.
> 2) Yes, in theory types should be legalized up
> to i16
> max, but for some reason i got that assertion
> triggered,
> i think it was with multibyte inline asm, so
> this has to
> be solved.
>
>
> 2013/6/28 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
>
> Hello Stepan, I will take a look to your patch
> later, but some questions:
>
> 1) does avr-gcc support 64bit values?
> 2) now that you're working with big data
> types, does
> it make sense to remove the assert I
> commented out
> about value types that are different to i8
> AND i16?
>
>
>
> 2013/6/28 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
>
> Hi Borja.
> This is almost the final multibyte
> constraint
> patch. Tests are not created yet.
>
> Currently I have issue with 64 bit
> values. Even
> if I pass them as inline asm operands, llvm
> still truncates them to i32.
>
> What works:
>
> long a; // 32 bit
> void f() {
> asm("instr %A0 %B0 %C0 %D0": : "r"(a));
> }
>
>
> What doesn't work:
>
> long long a; // 64 bit
> void f() {
> asm("instr %A0 %B0 %E0": : "r"(a));
> }
>
> The last one fires assertion since llvm
> allocates registers for the first 32
> bits only.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Yes fine
>
> El jueves, 27 de junio de 2013, Stepan
> Dyatkovskiy escribió:
>
> OK. Will do.
> Currently there is a patch
> with memory
> constraint and draft
> multibyte constraint (supports
> only A
> and B). To be clean under
> multibyte constraints I mean
> something
> like this:
> asm volatile("mov __tmp_reg__,
> %A0" "\n\t"
> "mov %A0, %B0"
> "\n\t"
> "mov %B0,
> __tmp_reg__" "\n\t"
> : "=r" (value)
> : "0" (value)
> );
>
> -Stepan.
> Borja Ferrer wrote:
>
> btw, if this is now feature
> complete you can commit it.
>
>
> 2013/6/27 Borja Ferrer
> <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
> No, that code is a
> personal
> modification of your examples.
> If you
> aren't getting the
> correct
> output from clang that's because you
> probably didn't apply
> a patch
> i commited last week for
> clang in our SVN.
> The assertion you're
> getting
> looks reasonable, it's an old
> friend of
> mine. The reason is
> that if Y
> is being reserved as the frame
> pointer, you cant have an
> instruction that uses 2 registers
> when
> only Z is available. I
> wouldn't mind too much about it.
>
> Now, my 2nd question:
>
> Is there anything
> else that
> needs to be covered for the memory
> constraint?
>
>
> 2013/6/27 Stepan
> Dyatkovskiy
> <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> This one looks good.
> Though ./llc emits "run out of
> registers
> allocation. I'll
> look at
> it. Is that from avr-libc?
> -Stepan.
> Borja Ferrer wrote:
>
> Yes, and then I
> replied the following:
>
> For this C code:
>
> char delay_count;
> char aaa;
> uint8_t
> delay(unsigned
> char p)
> {
> uint8_t
> cnt;
> asm
> volatile (
>
> "inst %0,
> %1" "\n\t"
> : "=Q"
> (delay_count)
> :
> "Q" (aaa));
> return cnt;
> }
>
> Clang produces:
>
> define i8
> @delay(i8
> %p) #0 {
> entry:
> tail call
> void asm
> sideeffect "inst $0, $1\0A\09",
> "=*Q,*Q"(i8*
> @delay_count, i8*
> @aaa) #2, !srcloc !4
> ret i8 undef
> }
>
> notice the *Q
> constraints, so is there anything
> wrong in there?
>
> Is there
> anything else
> that needs to be covered for the
> memory
> constraint?
>
>
> 2013/6/27 Stepan
> Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> Hi Borja,
>
> >
> Stepan, reply
> in this thread to my 2
> questions again
> if the email
> got lost.
>
> My prev.
> reply:
>
>
> > Ahh,
> I didn't
> know avr-gcc was buggy on this
> feature,
> that's probably
> > the
> reason for
> why the code i pasted in my
> previous
> email didn't
> work.
> ...
> >
> What's wrong
> with clang? I fixed clang
> inline asm
> support back on
> >
> friday, so is
> there anything else that is
> broken?
> It
> should emit
> '*Q' instead of 'Q'. That's why
> code
> from prev. email
> didn't
> work, just
> compare -emit-llvm -S output of
> avr-clang with
> other
> backends,
>
> > About the
> getPointerRegClass function, what
> is it
> used for? Also,
> please
> > move the
> implementation to the cpp file
> instead of
> leaving it in
> the .h.
>
> Currently, it is
> used in registers coelescing
> pass.
> While being
> registers
> inflating, llvm lookups all register
> uses. If
> it found
> that
> register is
> used by inline-asm as memory
> operand,
> it requests
> the
> pointer class
> with this method (I think
> the largest
> one). But
> may be
> in future
> this method will be used in
> more cases.
>
> avr-gcc
> has buggy
> implementation of memory
> constraint,
> that's why
> avr-libs
> doesn't
> use it at all. We have a
> chance to be
> first here
>
> -Stepan
>
> Borja
> Ferrer wrote:
>
>
> Stepan, reply
> in this thread to my 2 questions
> again if the
>
> email got lost.
>
>
>
> 2013/6/27
> Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>__>
>
>
>
>
> No, your
> last email only says : "So,
> can I
> commit that
> memory
>
> constraint patch?"
>
>
>
> 2013/6/27 Stepan Dyatkovskiy
>
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>
>
>
>
>
> Hm... Didn't you get my previous
> mail? If
> not, I think,
> I have
>
> to
> change my mail box.
>
>
> -Stepan
>
>
>
> -------- Original Message --------
>
> Subject: Re: [avr-llvm-devel] Inline
> assembly. Mostly
> just
> a stub.
>
> Date: Tue, 25 Jun 2013 18:30:46 +0400
>
> From: Stepan Dyatkovskiy
>
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>
>
> To:
> Borja Ferrer
> <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>__>
>
> CC:
> avr-llvm
>
>
> <avr-llvm-devel@lists.__source__________forge.net
> <http://source________forge.net> <http://source______forge.net>
>
> <http://source____forge.net>
>
> <http://source__forge.net>
>
> <http://source <http://sourceforge.net>
>
>
>
>
>
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-18 22:48:11
|
Ok fine ;)
I've just commited a fix for the problem you mentioned before. Let me know
if there are other issues with the assembler.
About the multibyte stuff:
I've carried some tests and i managed to crash it when i used other
register constraints, probably related with your modification in
avriselowering.cpp
2013/7/18 Stepan Dyatkovskiy <stp...@na...>
> Hello Borja,
> Sorry, but I can present configure in Sunday only.
>
> I'll proceed to work on remained inline issues then.
>
> --
> Truly yours,
> Stepan Dyatkovskiy
>
>
> 18.07.2013, 18:33, "Borja Ferrer" <bor...@gm...>:
>
> Heh thanks for those tests. I know you're a bit eager to compile avrlibc
> :) but I would like first to get inline asm completed since it's the last
> big blocker to get things compilable.
> I'm going to try something I thought to see if your multibyte patch works
> correctly under certain circumstances.
>
> About your questions:
>
> 1) Yes, the main reason is that we use a patched version of the greedy
> regalloc, using others will produce wrong code. You can locally use that
> patch to get avrlibc compilable but the code produced can be wrong.
> 2) Can you attach the regenerated configure file so i can try it locally?
> The best thing would be if Eric applies it to their svn repo.
>
> I'm aware of this error, I will try to fix it today. Is this the only type
> of error you're getting?
>
>
>
>
> 2013/7/18 Stepan Dyatkovskiy <stp...@na...>
>
> Hi Borja,
> Well, ok. I will write several tests for it. Though I think also about
> avr-libc compilation. Today I've made two small fixes in this direction:
>
> 1. Are there any reasons to forbid default FastRA for -O0? Note, currently
> for -O0 it doesn't work at all. So I've temoporary restored default
> behaviour here:
> --- a/lib/Target/AVR/**AVRTargetMachine.cpp
> +++ b/lib/Target/AVR/**AVRTargetMachine.cpp
> @@ -140,6 +140,10 @@ bool AVRPassConfig::addPreEmitPass(**)
>
> FunctionPass *AVRPassConfig::**createTargetRegisterAllocator(**bool
> Optimized)
> {
> - // Unconditionally use our custom greedy register allocator.
> - return createGreedyRegisterAllocator(**);
> +// // Unconditionally use our custom greedy register allocator.
> +// return createGreedyRegisterAllocator(**);
> + if (Optimized)
> + return createGreedyRegisterAllocator(**);
> + else
> + return createFastRegisterAllocator();
> }
>
> 2. I have also fixed avr-libc configure.ac to allow avr*clang compilers
> too. Current ToT filters out everything that is not in avr*gcc mask.
>
> 3. I also have patch applied (with multibytes referencing), I sent you few
> days ago.
>
> After I did these three changes, configure script has been finished
> successfully. Though during compilation it fails with next error (assembly
> file is attached):
>
> /tmp/fclose-3936b6.s: Assembler messages:
> /tmp/fclose-3936b6.s:22: Error: can't resolve `0' {*ABS* section} -
> `__iob' {*UND* section}
> /tmp/fclose-3936b6.s:22: Error: expression too complex
> /tmp/fclose-3936b6.s:23: Error: can't resolve `0' {*ABS* section} -
> `__iob' {*UND* section}
> /tmp/fclose-3936b6.s:23: Error: expression too complex
>
> Note it was for -mmcu=avr5.
>
> P.S.:
> Sorry about, I'm not following inline-asm testing strictly. To motivate
> myself I need to see not only pedantic unit-test implementation but a real
> progress too.
>
> Full invocation log:
>
> stepan@stepan-Inspiron-N5010:/**media/stepan/workproj/llvm.**
> project/2-avr/avr-libc/trunk/**build.opt/avr/lib/avr5$
> /media/stepan/workproj/llvm.**project/2-avr/builds/llvm.**
> debug-avr-arm-x86_64.build/**Debug+Asserts/bin/avr-clang -###
> -DHAVE_CONFIG_H -I. -I../../../../avr-libc/avr/**lib/avr5 -I../../..
> -I../../../../avr-libc/common -I../../../../avr-libc/include
> -I../../../include -Wall -W -Wstrict-prototypes -mmcu=avr5
> -D__COMPILING_AVR_LIBC__ -mcall-prologues -Os -MT fclose.o -MD -MP -MF
> .deps/fclose.Tpo -c -o fclose.o ../../../../avr-libc/libc/**stdio/fclose.c
> clang version 3.4 (http://llvm.org/git/clang.git3aa29df37b140f9c6786b6863a0cac
> **195071b598) (http://llvm.org/git/llvm.git https://github.com/kaomoneus/*
> *avr-llvm.git <https://github.com/kaomoneus/avr-llvm.git>ea6c1ebfeac54abd1583e90e518bdf
> **3bab5e1fd5)
> Target: avr
> Thread model: posix
>
> "/media/stepan/workproj/llvm.**project/2-avr/builds/llvm.**
> debug-avr-arm-x86_64.build/**Debug+Asserts/bin/clang" "-cc1" "-triple"
> "avr" "-S" "-disable-free" "-main-file-name" "fclose.c"
> "-mrelocation-model" "static" "-mdisable-fp-elim" "-fmath-errno"
> "-mconstructor-aliases" "-target-linker-version" "2.22.90.20120924"
> "-coverage-file" "/tmp/fclose-29b051.s" "-resource-dir"
> "/media/stepan/workproj/llvm.**project/2-avr/builds/llvm.**
> debug-avr-arm-x86_64.build/**Debug+Asserts/bin/../lib/**clang/3.4"
> "-dependency-file" ".deps/fclose.Tpo" "-sys-header-deps" "-MP" "-MT"
> "fclose.o" "-D" "HAVE_CONFIG_H" "-D" "__COMPILING_AVR_LIBC__" "-I" "." "-I"
> "../../../../avr-libc/avr/lib/**avr5" "-I" "../../.." "-I"
> "../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I"
> "../../../include" "-Os" "-Wall" "-W" "-Wstrict-prototypes"
> "-fno-dwarf-directory-asm" "-fdebug-compilation-dir"
> "/media/stepan/workproj/llvm.**project/2-avr/avr-libc/trunk/**build.opt/avr/lib/avr5"
> "-ferror-limit" "19" "-fmessage-length" "147" "-mstackrealign"
> "-fobjc-runtime=gcc" "-fobjc-default-synthesize-**properties"
> "-fdiagnostics-show-option" "-fcolor-diagnostics" "-vectorize-loops" "-o"
> "/tmp/fclose-29b051.s" "-x" "c" "../../../../avr-libc/libc/**
> stdio/fclose.c"
> "/usr/bin/avr-gcc" "-D" "HAVE_CONFIG_H" "-I" "." "-I"
> "../../../../avr-libc/avr/lib/**avr5" "-I" "../../.." "-I"
> "../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I"
> "../../../include" "-Wall" "-W" "-Wstrict-prototypes" "-mmcu=avr5" "-D"
> "__COMPILING_AVR_LIBC__" "-mcall-prologues" "-Os" "-MT" "fclose.o" "-MD"
> "-MP" "-MF" ".deps/fclose.Tpo" "-c" "-o" "fclose.o" "-x" "assembler"
> "/tmp/fclose-29b051.s"
>
> Borja Ferrer wrote:
>
> Ok, if this will work with other reg constraints then add some tests to
> cover them aswell.
>
> What errors are you getting exactly with avrlibc? IIRC there's an assert
> in llc that fires when running the greedy regalloc in O0. I dont know
> the reason for this assert and if it can be safely removed, this has to
> be asked in the dev list and also get some possible solutions.
>
> 2013/7/17 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
> >>
>
>
> Hi Borja.
> It should work for all other constraints as well. I don't like idea
> of fixing clang too. So we can just update
> getRegForInlineAsmConstraint method.
>
> I'd tried to compile avr-libc. Currently clang doesn't want to
> compile everything with -O0. While configure script tests everything
> with -O0. Do you have any ideas how to fix -O0 ?
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Also, I guess doing only this for r is not enough, you can use
> multibytes with other register constraints.
>
>
> 2013/7/16 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm... <mailto:bor...@gm...>**>__>
>
>
>
> What are you suggesting to do in clang?
>
>
> 2013/7/16 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>
>
>
>
> Back to multibyte referenced.
>
> I've attached patch with this feature + unit-test.
> There is some
> question though.
> Currently, .c code:
> int a;
> void f() {
> asm("instr %A0 %B0": : "r"(a));
> }
> is transformed into IR below:
> define void @f(i16 %a) {
> entry:
> call void asm sideeffect "instr ${0:A} ${0:B}",
> "r"(i16 %a)
> ret void
> }
> To prevent the crash I had to fix
> getRegForInlineAsmConstraint,
> I set it to return DREGS when it see i16 type or
> bigger. Though,
> may be you want to fix clang and replace "r" constraint
> with
> another one.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Just remembered after hitting resend that this has
> to be
> done always,
> similarly like in SelectAddr(). However in this
> case it can
> be done
> unconditionally since imm is guaranteed to fit in
> 6bits, so
> even if it
> comes in a constant node of type i64 it's safe to
> truncate
> it to i8.
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
>
> <mailto:stp...@na...>
>
> <mailto:stp...@na... <mailto:stp...@na...
> >>>>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue()._____**_getZExtValue()
>
> < 64;
>
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed
> because
> i found it
> redundant:
> > + if (ImmNode->getValueType(0) !=
> MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > +
> ImmNode->getAPIntValue()._____**_getZExtValue(),
>
> MVT::i8);
>
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp",
> and got
> the crash. I
> don't remember what crash says exactly.
> Currently, I'm
> not able to
> compile anything. What I had found out is that
> Disp (aka
> displacement) should be i8 only. Our back-end
> just
> don't expect
> other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Thanks!
>
> I've been cleaning a bit the code, and fixed
> a
> possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue()._____**_getZExtValue() < 64;
>
>
>
> Since ImmNode comes from a dyncast, if
> it's NULL
> the second line
> would crash.
>
>
>
> There's a piece of code i initially
> removed because
> i found it
> redundant:
> + if (ImmNode->getValueType(0) !=
> MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> +
> ImmNode->getAPIntValue()._____**_getZExtValue(),
>
> MVT::i8);
>
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you
> explain why
> this happens
> and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>> <mailto:stp...@na...
>
> <mailto:stp...@na...>
>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better,
> atleast fixing
> patches
> iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests
> with the
> machine
> verifier enabled
> then
> feel free to commit it. I'll take
> a look
> at the glue
> operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...
> >>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...
> >>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>**>
>
>
> Hello Borja.
> Seems I've made things much
> simpler.
> Can't
> understand why I
> didn't
> it in beginning.
>
> I moved everything into
> SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at
> all. Since it
> implemented in
> hook caller.
> #2: This problem
> disappeared, see #1.
> #4: The same.
> #6: Still not sure about
> glues. I
> just tried to
> keep all
> chains in
> proper order. But you rather
> look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you
> run the
> regression tests,
> enable locally the
> > machine verifier to catch
> any
> additional errors. To
> enable it
> > unconditionally search
> for the
> command object
> (i think it's
> declared in
> > codegen/passes.cpp as a
> cl::opt)
> and turn it on by
> default. When
> you're
> > doing your own tests you can
> enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really
> big
> conditions xD.
> You could
> split them
> into
> single conditions and
> use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge
> if()
> that is quite
> hard to read.
>
> 2) Yes I thought about that
> aswell, that not all
> constant nodes
> are asm
> flags, but the ARM
> backend does
> this, so
> either they
> have a bug
> there or
> it's safe?
> I prefer if you could
> move the
> big condition
> inside the
> for()
> into the
> top of the loop, it
> makes the
> for() quite
> unreadable.
> And adjust
> there
> the i variable as needed
> for each
> suboperand
> as you
> mentioned
> before.
>
> 4) Why do you say they
> handle
> everything in the
> SelectInlineAsm
> method?
> As far as i can see they
> only
> handle one
> specific case,
> otherwise they
> return NULL and let the
> default
> behaviour do
> the work.
>
> 6) Yes glues are a bit
> tricky,
> dont worry
> about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you
> run the
> regression
> tests, enable
> locally the
> machine verifier to
> catch any
> additional
> errors. To
> enable it
> unconditionally search
> for the
> command object
> (i think it's
> declared in
> codegen/passes.cpp as a
> cl::opt)
> and turn it on by
> default. When
> you're
> doing your own tests you
> can
> enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
>
>
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-18 14:33:35
|
Heh thanks for those tests. I know you're a bit eager to compile avrlibc :)
but I would like first to get inline asm completed since it's the last big
blocker to get things compilable.
I'm going to try something I thought to see if your multibyte patch works
correctly under certain circumstances.
About your questions:
1) Yes, the main reason is that we use a patched version of the greedy
regalloc, using others will produce wrong code. You can locally use that
patch to get avrlibc compilable but the code produced can be wrong.
2) Can you attach the regenerated configure file so i can try it locally?
The best thing would be if Eric applies it to their svn repo.
I'm aware of this error, I will try to fix it today. Is this the only type
of error you're getting?
2013/7/18 Stepan Dyatkovskiy <stp...@na...>
> Hi Borja,
> Well, ok. I will write several tests for it. Though I think also about
> avr-libc compilation. Today I've made two small fixes in this direction:
>
> 1. Are there any reasons to forbid default FastRA for -O0? Note, currently
> for -O0 it doesn't work at all. So I've temoporary restored default
> behaviour here:
> --- a/lib/Target/AVR/**AVRTargetMachine.cpp
> +++ b/lib/Target/AVR/**AVRTargetMachine.cpp
> @@ -140,6 +140,10 @@ bool AVRPassConfig::addPreEmitPass(**)
>
> FunctionPass *AVRPassConfig::**createTargetRegisterAllocator(**bool
> Optimized)
> {
> - // Unconditionally use our custom greedy register allocator.
> - return createGreedyRegisterAllocator(**);
> +// // Unconditionally use our custom greedy register allocator.
> +// return createGreedyRegisterAllocator(**);
> + if (Optimized)
> + return createGreedyRegisterAllocator(**);
> + else
> + return createFastRegisterAllocator();
> }
>
> 2. I have also fixed avr-libc configure.ac to allow avr*clang compilers
> too. Current ToT filters out everything that is not in avr*gcc mask.
>
> 3. I also have patch applied (with multibytes referencing), I sent you few
> days ago.
>
> After I did these three changes, configure script has been finished
> successfully. Though during compilation it fails with next error (assembly
> file is attached):
>
> /tmp/fclose-3936b6.s: Assembler messages:
> /tmp/fclose-3936b6.s:22: Error: can't resolve `0' {*ABS* section} -
> `__iob' {*UND* section}
> /tmp/fclose-3936b6.s:22: Error: expression too complex
> /tmp/fclose-3936b6.s:23: Error: can't resolve `0' {*ABS* section} -
> `__iob' {*UND* section}
> /tmp/fclose-3936b6.s:23: Error: expression too complex
>
> Note it was for -mmcu=avr5.
>
> P.S.:
> Sorry about, I'm not following inline-asm testing strictly. To motivate
> myself I need to see not only pedantic unit-test implementation but a real
> progress too.
>
> Full invocation log:
>
> stepan@stepan-Inspiron-N5010:/**media/stepan/workproj/llvm.**
> project/2-avr/avr-libc/trunk/**build.opt/avr/lib/avr5$
> /media/stepan/workproj/llvm.**project/2-avr/builds/llvm.**
> debug-avr-arm-x86_64.build/**Debug+Asserts/bin/avr-clang -###
> -DHAVE_CONFIG_H -I. -I../../../../avr-libc/avr/**lib/avr5 -I../../..
> -I../../../../avr-libc/common -I../../../../avr-libc/include
> -I../../../include -Wall -W -Wstrict-prototypes -mmcu=avr5
> -D__COMPILING_AVR_LIBC__ -mcall-prologues -Os -MT fclose.o -MD -MP -MF
> .deps/fclose.Tpo -c -o fclose.o ../../../../avr-libc/libc/**stdio/fclose.c
> clang version 3.4 (http://llvm.org/git/clang.git3aa29df37b140f9c6786b6863a0cac
> **195071b598) (http://llvm.org/git/llvm.git https://github.com/kaomoneus/*
> *avr-llvm.git <https://github.com/kaomoneus/avr-llvm.git>ea6c1ebfeac54abd1583e90e518bdf
> **3bab5e1fd5)
> Target: avr
> Thread model: posix
>
> "/media/stepan/workproj/llvm.**project/2-avr/builds/llvm.**
> debug-avr-arm-x86_64.build/**Debug+Asserts/bin/clang" "-cc1" "-triple"
> "avr" "-S" "-disable-free" "-main-file-name" "fclose.c"
> "-mrelocation-model" "static" "-mdisable-fp-elim" "-fmath-errno"
> "-mconstructor-aliases" "-target-linker-version" "2.22.90.20120924"
> "-coverage-file" "/tmp/fclose-29b051.s" "-resource-dir"
> "/media/stepan/workproj/llvm.**project/2-avr/builds/llvm.**
> debug-avr-arm-x86_64.build/**Debug+Asserts/bin/../lib/**clang/3.4"
> "-dependency-file" ".deps/fclose.Tpo" "-sys-header-deps" "-MP" "-MT"
> "fclose.o" "-D" "HAVE_CONFIG_H" "-D" "__COMPILING_AVR_LIBC__" "-I" "." "-I"
> "../../../../avr-libc/avr/lib/**avr5" "-I" "../../.." "-I"
> "../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I"
> "../../../include" "-Os" "-Wall" "-W" "-Wstrict-prototypes"
> "-fno-dwarf-directory-asm" "-fdebug-compilation-dir"
> "/media/stepan/workproj/llvm.**project/2-avr/avr-libc/trunk/**build.opt/avr/lib/avr5"
> "-ferror-limit" "19" "-fmessage-length" "147" "-mstackrealign"
> "-fobjc-runtime=gcc" "-fobjc-default-synthesize-**properties"
> "-fdiagnostics-show-option" "-fcolor-diagnostics" "-vectorize-loops" "-o"
> "/tmp/fclose-29b051.s" "-x" "c" "../../../../avr-libc/libc/**
> stdio/fclose.c"
> "/usr/bin/avr-gcc" "-D" "HAVE_CONFIG_H" "-I" "." "-I"
> "../../../../avr-libc/avr/lib/**avr5" "-I" "../../.." "-I"
> "../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I"
> "../../../include" "-Wall" "-W" "-Wstrict-prototypes" "-mmcu=avr5" "-D"
> "__COMPILING_AVR_LIBC__" "-mcall-prologues" "-Os" "-MT" "fclose.o" "-MD"
> "-MP" "-MF" ".deps/fclose.Tpo" "-c" "-o" "fclose.o" "-x" "assembler"
> "/tmp/fclose-29b051.s"
>
> Borja Ferrer wrote:
>
>> Ok, if this will work with other reg constraints then add some tests to
>> cover them aswell.
>>
>> What errors are you getting exactly with avrlibc? IIRC there's an assert
>> in llc that fires when running the greedy regalloc in O0. I dont know
>> the reason for this assert and if it can be safely removed, this has to
>> be asked in the dev list and also get some possible solutions.
>>
>>
>> 2013/7/17 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
>> >>
>>
>>
>> Hi Borja.
>> It should work for all other constraints as well. I don't like idea
>> of fixing clang too. So we can just update
>> getRegForInlineAsmConstraint method.
>>
>> I'd tried to compile avr-libc. Currently clang doesn't want to
>> compile everything with -O0. While configure script tests everything
>> with -O0. Do you have any ideas how to fix -O0 ?
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>> Also, I guess doing only this for r is not enough, you can use
>> multibytes with other register constraints.
>>
>>
>> 2013/7/16 Borja Ferrer <bor...@gm...
>> <mailto:bor...@gm...>
>> <mailto:bor...@gm... <mailto:bor...@gm...>**
>> >__>
>>
>>
>>
>> What are you suggesting to do in clang?
>>
>>
>> 2013/7/16 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...>>>
>>
>>
>>
>> Back to multibyte referenced.
>>
>> I've attached patch with this feature + unit-test.
>> There is some
>> question though.
>> Currently, .c code:
>> int a;
>> void f() {
>> asm("instr %A0 %B0": : "r"(a));
>> }
>> is transformed into IR below:
>> define void @f(i16 %a) {
>> entry:
>> call void asm sideeffect "instr ${0:A} ${0:B}",
>> "r"(i16 %a)
>> ret void
>> }
>> To prevent the crash I had to fix
>> getRegForInlineAsmConstraint,
>> I set it to return DREGS when it see i16 type or
>> bigger. Though,
>> may be you want to fix clang and replace "r" constraint
>> with
>> another one.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>> Just remembered after hitting resend that this has
>> to be
>> done always,
>> similarly like in SelectAddr(). However in this
>> case it can
>> be done
>> unconditionally since imm is guaranteed to fit in
>> 6bits, so
>> even if it
>> comes in a constant node of type i64 it's safe to
>> truncate
>> it to i8.
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>> <mailto:stp...@na...
>>
>> <mailto:stp...@na...>
>>
>> <mailto:stp...@na... <mailto:stp...@na...
>> >>>>
>>
>> Hi Borja,
>>
>>
>> > + CanHandleRegImmOpt &= ImmNode != 0;
>> > + CanHandleRegImmOpt &=
>> ImmNode->getAPIntValue()._____**_getZExtValue()
>>
>> < 64;
>>
>>
>> You absolutely right here. Thanks.
>>
>>
>> > There's a piece of code i initially removed
>> because
>> i found it
>> redundant:
>> > + if (ImmNode->getValueType(0) !=
>> MVT::i8)
>> > + {
>> > + Disp = CurDAG->getTargetConstant(
>> > +
>> ImmNode->getAPIntValue()._____**_getZExtValue(),
>>
>> MVT::i8);
>>
>> > + }
>> > + else
>> > + {
>> > + Disp = ImmOp;
>> > + }
>>
>> Initially did the same: just "Disp = ImmOp",
>> and got
>> the crash. I
>> don't remember what crash says exactly.
>> Currently, I'm
>> not able to
>> compile anything. What I had found out is that
>> Disp (aka
>> displacement) should be i8 only. Our back-end
>> just
>> don't expect
>> other types of displacements.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>> Thanks!
>>
>> I've been cleaning a bit the code, and
>> fixed a
>> possible crash here:
>>
>> + CanHandleRegImmOpt &= ImmNode != 0;
>> + CanHandleRegImmOpt &=
>>
>> ImmNode->getAPIntValue()._____**_getZExtValue() < 64;
>>
>>
>>
>> Since ImmNode comes from a dyncast, if
>> it's NULL
>> the second line
>> would crash.
>>
>>
>>
>> There's a piece of code i initially
>> removed because
>> i found it
>> redundant:
>> + if (ImmNode->getValueType(0) !=
>> MVT::i8)
>> + {
>> + Disp = CurDAG->getTargetConstant(
>> +
>> ImmNode->getAPIntValue()._____**_getZExtValue(),
>>
>> MVT::i8);
>>
>> + }
>> + else
>> + {
>> + Disp = ImmOp;
>> + }
>>
>> but without im getting crashes. Can you
>> explain why
>> this happens
>> and is needed?
>>
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy
>> <stp...@na... <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...
>> >>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>>> <mailto:stp...@na...
>>
>> <mailto:stp...@na...>
>>
>> <mailto:stp...@na... <mailto:stp...@na...
>> >>
>> <mailto:stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>> <mailto:stp...@na...>>>>>
>>
>>
>> r269
>> -Stepan
>> Borja Ferrer wrote:
>>
>> Ohhh this is a lot better,
>> atleast fixing
>> patches
>> iteratively
>> brought us
>> to a nice solution :)
>>
>> If this patch passes all tests
>> with the
>> machine
>> verifier enabled
>> then
>> feel free to commit it. I'll take
>> a look
>> at the glue
>> operands
>> after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy
>> <stp...@na... <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...>>
>> <mailto:stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>> <mailto:stp...@na...>>>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>> <mailto:stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...
>> >>>>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>> <mailto:stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...
>> >>>
>>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>> <mailto:stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>>>>>**>
>>
>>
>> Hello Borja.
>> Seems I've made things much
>> simpler.
>> Can't
>> understand why I
>> didn't
>> it in beginning.
>>
>> I moved everything into
>> SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at
>> all. Since it
>> implemented in
>> hook caller.
>> #2: This problem
>> disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about
>> glues. I
>> just tried to
>> keep all
>> chains in
>> proper order. But you rather
>> look at new
>> SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you
>> run the
>> regression tests,
>> enable locally the
>> > machine verifier to catch
>> any
>> additional errors. To
>> enable it
>> > unconditionally search
>> for the
>> command object
>> (i think it's
>> declared in
>> > codegen/passes.cpp as a
>> cl::opt)
>> and turn it on by
>> default. When
>> you're
>> > doing your own tests you
>> can
>> enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really
>> big
>> conditions xD.
>> You could
>> split them
>> into
>> single conditions and
>> use continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge
>> if()
>> that is quite
>> hard to read.
>>
>> 2) Yes I thought about
>> that
>> aswell, that not all
>> constant nodes
>> are asm
>> flags, but the ARM
>> backend does
>> this, so
>> either they
>> have a bug
>> there or
>> it's safe?
>> I prefer if you could
>> move the
>> big condition
>> inside the
>> for()
>> into the
>> top of the loop, it
>> makes the
>> for() quite
>> unreadable.
>> And adjust
>> there
>> the i variable as needed
>> for each
>> suboperand
>> as you
>> mentioned
>> before.
>>
>> 4) Why do you say they
>> handle
>> everything in the
>> SelectInlineAsm
>> method?
>> As far as i can see they
>> only
>> handle one
>> specific case,
>> otherwise they
>> return NULL and let the
>> default
>> behaviour do
>> the work.
>>
>> 6) Yes glues are a bit
>> tricky,
>> dont worry
>> about them, I
>> will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you
>> run the
>> regression
>> tests, enable
>> locally the
>> machine verifier to
>> catch any
>> additional
>> errors. To
>> enable it
>> unconditionally search
>> for the
>> command object
>> (i think it's
>> declared in
>> codegen/passes.cpp as a
>> cl::opt)
>> and turn it on by
>> default. When
>> you're
>> doing your own tests you
>> can
>> enable it passing
>> -verify-machineisntrs or
>> something like that to
>> llc.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-18 11:12:16
|
Hi Borja,
Well, ok. I will write several tests for it. Though I think also about
avr-libc compilation. Today I've made two small fixes in this direction:
1. Are there any reasons to forbid default FastRA for -O0? Note,
currently for -O0 it doesn't work at all. So I've temoporary restored
default behaviour here:
--- a/lib/Target/AVR/AVRTargetMachine.cpp
+++ b/lib/Target/AVR/AVRTargetMachine.cpp
@@ -140,6 +140,10 @@ bool AVRPassConfig::addPreEmitPass()
FunctionPass *AVRPassConfig::createTargetRegisterAllocator(bool Optimized)
{
- // Unconditionally use our custom greedy register allocator.
- return createGreedyRegisterAllocator();
+// // Unconditionally use our custom greedy register allocator.
+// return createGreedyRegisterAllocator();
+ if (Optimized)
+ return createGreedyRegisterAllocator();
+ else
+ return createFastRegisterAllocator();
}
2. I have also fixed avr-libc configure.ac to allow avr*clang compilers
too. Current ToT filters out everything that is not in avr*gcc mask.
3. I also have patch applied (with multibytes referencing), I sent you
few days ago.
After I did these three changes, configure script has been finished
successfully. Though during compilation it fails with next error
(assembly file is attached):
/tmp/fclose-3936b6.s: Assembler messages:
/tmp/fclose-3936b6.s:22: Error: can't resolve `0' {*ABS* section} -
`__iob' {*UND* section}
/tmp/fclose-3936b6.s:22: Error: expression too complex
/tmp/fclose-3936b6.s:23: Error: can't resolve `0' {*ABS* section} -
`__iob' {*UND* section}
/tmp/fclose-3936b6.s:23: Error: expression too complex
Note it was for -mmcu=avr5.
P.S.:
Sorry about, I'm not following inline-asm testing strictly. To motivate
myself I need to see not only pedantic unit-test implementation but a
real progress too.
Full invocation log:
stepan@stepan-Inspiron-N5010:/media/stepan/workproj/llvm.project/2-avr/avr-libc/trunk/build.opt/avr/lib/avr5$
/media/stepan/workproj/llvm.project/2-avr/builds/llvm.debug-avr-arm-x86_64.build/Debug+Asserts/bin/avr-clang
-### -DHAVE_CONFIG_H -I. -I../../../../avr-libc/avr/lib/avr5 -I../../..
-I../../../../avr-libc/common -I../../../../avr-libc/include
-I../../../include -Wall -W -Wstrict-prototypes -mmcu=avr5
-D__COMPILING_AVR_LIBC__ -mcall-prologues -Os -MT fclose.o -MD -MP -MF
.deps/fclose.Tpo -c -o fclose.o ../../../../avr-libc/libc/stdio/fclose.c
clang version 3.4 (http://llvm.org/git/clang.git
3aa29df37b140f9c6786b6863a0cac195071b598) (http://llvm.org/git/llvm.git
https://github.com/kaomoneus/avr-llvm.git
ea6c1ebfeac54abd1583e90e518bdf3bab5e1fd5)
Target: avr
Thread model: posix
"/media/stepan/workproj/llvm.project/2-avr/builds/llvm.debug-avr-arm-x86_64.build/Debug+Asserts/bin/clang"
"-cc1" "-triple" "avr" "-S" "-disable-free" "-main-file-name" "fclose.c"
"-mrelocation-model" "static" "-mdisable-fp-elim" "-fmath-errno"
"-mconstructor-aliases" "-target-linker-version" "2.22.90.20120924"
"-coverage-file" "/tmp/fclose-29b051.s" "-resource-dir"
"/media/stepan/workproj/llvm.project/2-avr/builds/llvm.debug-avr-arm-x86_64.build/Debug+Asserts/bin/../lib/clang/3.4"
"-dependency-file" ".deps/fclose.Tpo" "-sys-header-deps" "-MP" "-MT"
"fclose.o" "-D" "HAVE_CONFIG_H" "-D" "__COMPILING_AVR_LIBC__" "-I" "."
"-I" "../../../../avr-libc/avr/lib/avr5" "-I" "../../.." "-I"
"../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I"
"../../../include" "-Os" "-Wall" "-W" "-Wstrict-prototypes"
"-fno-dwarf-directory-asm" "-fdebug-compilation-dir"
"/media/stepan/workproj/llvm.project/2-avr/avr-libc/trunk/build.opt/avr/lib/avr5"
"-ferror-limit" "19" "-fmessage-length" "147" "-mstackrealign"
"-fobjc-runtime=gcc" "-fobjc-default-synthesize-properties"
"-fdiagnostics-show-option" "-fcolor-diagnostics" "-vectorize-loops"
"-o" "/tmp/fclose-29b051.s" "-x" "c"
"../../../../avr-libc/libc/stdio/fclose.c"
"/usr/bin/avr-gcc" "-D" "HAVE_CONFIG_H" "-I" "." "-I"
"../../../../avr-libc/avr/lib/avr5" "-I" "../../.." "-I"
"../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I"
"../../../include" "-Wall" "-W" "-Wstrict-prototypes" "-mmcu=avr5" "-D"
"__COMPILING_AVR_LIBC__" "-mcall-prologues" "-Os" "-MT" "fclose.o" "-MD"
"-MP" "-MF" ".deps/fclose.Tpo" "-c" "-o" "fclose.o" "-x" "assembler"
"/tmp/fclose-29b051.s"
Borja Ferrer wrote:
> Ok, if this will work with other reg constraints then add some tests to
> cover them aswell.
>
> What errors are you getting exactly with avrlibc? IIRC there's an assert
> in llc that fires when running the greedy regalloc in O0. I dont know
> the reason for this assert and if it can be safely removed, this has to
> be asked in the dev list and also get some possible solutions.
>
>
> 2013/7/17 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>>
>
> Hi Borja.
> It should work for all other constraints as well. I don't like idea
> of fixing clang too. So we can just update
> getRegForInlineAsmConstraint method.
>
> I'd tried to compile avr-libc. Currently clang doesn't want to
> compile everything with -O0. While configure script tests everything
> with -O0. Do you have any ideas how to fix -O0 ?
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Also, I guess doing only this for r is not enough, you can use
> multibytes with other register constraints.
>
>
> 2013/7/16 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm... <mailto:bor...@gm...>>__>
>
>
> What are you suggesting to do in clang?
>
>
> 2013/7/16 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>
>
>
> Back to multibyte referenced.
>
> I've attached patch with this feature + unit-test.
> There is some
> question though.
> Currently, .c code:
> int a;
> void f() {
> asm("instr %A0 %B0": : "r"(a));
> }
> is transformed into IR below:
> define void @f(i16 %a) {
> entry:
> call void asm sideeffect "instr ${0:A} ${0:B}",
> "r"(i16 %a)
> ret void
> }
> To prevent the crash I had to fix
> getRegForInlineAsmConstraint,
> I set it to return DREGS when it see i16 type or
> bigger. Though,
> may be you want to fix clang and replace "r" constraint
> with
> another one.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Just remembered after hitting resend that this has
> to be
> done always,
> similarly like in SelectAddr(). However in this
> case it can
> be done
> unconditionally since imm is guaranteed to fit in
> 6bits, so
> even if it
> comes in a constant node of type i64 it's safe to
> truncate
> it to i8.
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
>
> <mailto:stp...@na... <mailto:stp...@na...>>>>
>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().______getZExtValue()
> < 64;
>
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed
> because
> i found it
> redundant:
> > + if (ImmNode->getValueType(0) != MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > +
> ImmNode->getAPIntValue().______getZExtValue(),
> MVT::i8);
>
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp",
> and got
> the crash. I
> don't remember what crash says exactly.
> Currently, I'm
> not able to
> compile anything. What I had found out is that
> Disp (aka
> displacement) should be i8 only. Our back-end just
> don't expect
> other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Thanks!
>
> I've been cleaning a bit the code, and fixed a
> possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &=
>
> ImmNode->getAPIntValue().______getZExtValue() < 64;
>
>
> Since ImmNode comes from a dyncast, if
> it's NULL
> the second line
> would crash.
>
>
>
> There's a piece of code i initially
> removed because
> i found it
> redundant:
> + if (ImmNode->getValueType(0) !=
> MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> +
> ImmNode->getAPIntValue().______getZExtValue(),
> MVT::i8);
>
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you
> explain why
> this happens
> and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>> <mailto:stp...@na...
> <mailto:stp...@na...>
>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better,
> atleast fixing
> patches
> iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests
> with the
> machine
> verifier enabled
> then
> feel free to commit it. I'll take
> a look
> at the glue
> operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>
>
>
> Hello Borja.
> Seems I've made things much
> simpler.
> Can't
> understand why I
> didn't
> it in beginning.
>
> I moved everything into
> SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at
> all. Since it
> implemented in
> hook caller.
> #2: This problem
> disappeared, see #1.
> #4: The same.
> #6: Still not sure about
> glues. I
> just tried to
> keep all
> chains in
> proper order. But you rather
> look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you
> run the
> regression tests,
> enable locally the
> > machine verifier to catch any
> additional errors. To
> enable it
> > unconditionally search
> for the
> command object
> (i think it's
> declared in
> > codegen/passes.cpp as a
> cl::opt)
> and turn it on by
> default. When
> you're
> > doing your own tests you can
> enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really big
> conditions xD.
> You could
> split them
> into
> single conditions and
> use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge
> if()
> that is quite
> hard to read.
>
> 2) Yes I thought about that
> aswell, that not all
> constant nodes
> are asm
> flags, but the ARM
> backend does
> this, so
> either they
> have a bug
> there or
> it's safe?
> I prefer if you could
> move the
> big condition
> inside the
> for()
> into the
> top of the loop, it
> makes the
> for() quite
> unreadable.
> And adjust
> there
> the i variable as needed
> for each
> suboperand
> as you
> mentioned
> before.
>
> 4) Why do you say they
> handle
> everything in the
> SelectInlineAsm
> method?
> As far as i can see they
> only
> handle one
> specific case,
> otherwise they
> return NULL and let the
> default
> behaviour do
> the work.
>
> 6) Yes glues are a bit
> tricky,
> dont worry
> about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you
> run the
> regression
> tests, enable
> locally the
> machine verifier to
> catch any
> additional
> errors. To
> enable it
> unconditionally search
> for the
> command object
> (i think it's
> declared in
> codegen/passes.cpp as a
> cl::opt)
> and turn it on by
> default. When
> you're
> doing your own tests you can
> enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
>
>
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-17 14:07:39
|
Ok, if this will work with other reg constraints then add some tests to
cover them aswell.
What errors are you getting exactly with avrlibc? IIRC there's an assert in
llc that fires when running the greedy regalloc in O0. I dont know the
reason for this assert and if it can be safely removed, this has to be
asked in the dev list and also get some possible solutions.
2013/7/17 Stepan Dyatkovskiy <stp...@na...>
> Hi Borja.
> It should work for all other constraints as well. I don't like idea of
> fixing clang too. So we can just update getRegForInlineAsmConstraint method.
>
> I'd tried to compile avr-libc. Currently clang doesn't want to compile
> everything with -O0. While configure script tests everything with -O0. Do
> you have any ideas how to fix -O0 ?
>
> -Stepan.
>
> Borja Ferrer wrote:
>
>> Also, I guess doing only this for r is not enough, you can use
>> multibytes with other register constraints.
>>
>>
>> 2013/7/16 Borja Ferrer <bor...@gm...
>> <mailto:bor...@gm...>**>
>>
>>
>> What are you suggesting to do in clang?
>>
>>
>> 2013/7/16 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>>
>>
>>
>> Back to multibyte referenced.
>>
>> I've attached patch with this feature + unit-test. There is some
>> question though.
>> Currently, .c code:
>> int a;
>> void f() {
>> asm("instr %A0 %B0": : "r"(a));
>> }
>> is transformed into IR below:
>> define void @f(i16 %a) {
>> entry:
>> call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
>> ret void
>> }
>> To prevent the crash I had to fix getRegForInlineAsmConstraint,
>> I set it to return DREGS when it see i16 type or bigger. Though,
>> may be you want to fix clang and replace "r" constraint with
>> another one.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>> Just remembered after hitting resend that this has to be
>> done always,
>> similarly like in SelectAddr(). However in this case it can
>> be done
>> unconditionally since imm is guaranteed to fit in 6bits, so
>> even if it
>> comes in a constant node of type i64 it's safe to truncate
>> it to i8.
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>>
>> <mailto:stp...@na...>>>
>>
>> Hi Borja,
>>
>>
>> > + CanHandleRegImmOpt &= ImmNode != 0;
>> > + CanHandleRegImmOpt &=
>> ImmNode->getAPIntValue().____**getZExtValue() < 64;
>>
>>
>> You absolutely right here. Thanks.
>>
>>
>> > There's a piece of code i initially removed because
>> i found it
>> redundant:
>> > + if (ImmNode->getValueType(0) != MVT::i8)
>> > + {
>> > + Disp = CurDAG->getTargetConstant(
>> > +
>> ImmNode->getAPIntValue().____**getZExtValue(), MVT::i8);
>>
>> > + }
>> > + else
>> > + {
>> > + Disp = ImmOp;
>> > + }
>>
>> Initially did the same: just "Disp = ImmOp", and got
>> the crash. I
>> don't remember what crash says exactly. Currently, I'm
>> not able to
>> compile anything. What I had found out is that Disp (aka
>> displacement) should be i8 only. Our back-end just
>> don't expect
>> other types of displacements.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>> Thanks!
>>
>> I've been cleaning a bit the code, and fixed a
>> possible crash here:
>>
>> + CanHandleRegImmOpt &= ImmNode != 0;
>> + CanHandleRegImmOpt &=
>> ImmNode->getAPIntValue().____**getZExtValue() < 64;
>>
>>
>> Since ImmNode comes from a dyncast, if it's NULL
>> the second line
>> would crash.
>>
>>
>>
>> There's a piece of code i initially removed because
>> i found it
>> redundant:
>> + if (ImmNode->getValueType(0) != MVT::i8)
>> + {
>> + Disp = CurDAG->getTargetConstant(
>> +
>> ImmNode->getAPIntValue().____**getZExtValue(), MVT::i8);
>>
>> + }
>> + else
>> + {
>> + Disp = ImmOp;
>> + }
>>
>> but without im getting crashes. Can you explain why
>> this happens
>> and is needed?
>>
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na...
>> <mailto:stp...@na...>> <mailto:stp...@na...
>>
>> <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...
>> >>>>
>>
>>
>> r269
>> -Stepan
>> Borja Ferrer wrote:
>>
>> Ohhh this is a lot better, atleast fixing
>> patches
>> iteratively
>> brought us
>> to a nice solution :)
>>
>> If this patch passes all tests with the
>> machine
>> verifier enabled
>> then
>> feel free to commit it. I'll take a look
>> at the glue
>> operands
>> after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy
>> <stp...@na... <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...
>> >>
>> <mailto:stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>> <mailto:stp...@na...>>>
>> <mailto:stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>> <mailto:stp...@na...>>
>>
>> <mailto:stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>> <mailto:stp...@na...>>>>>
>>
>>
>> Hello Borja.
>> Seems I've made things much simpler.
>> Can't
>> understand why I
>> didn't
>> it in beginning.
>>
>> I moved everything into
>> SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at all. Since
>> it
>> implemented in
>> hook caller.
>> #2: This problem disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about glues. I
>> just tried to
>> keep all
>> chains in
>> proper order. But you rather look at
>> new
>> SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you run the
>> regression tests,
>> enable locally the
>> > machine verifier to catch any
>> additional errors. To
>> enable it
>> > unconditionally search for the
>> command object
>> (i think it's
>> declared in
>> > codegen/passes.cpp as a cl::opt)
>> and turn it on by
>> default. When
>> you're
>> > doing your own tests you can
>> enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really big
>> conditions xD.
>> You could
>> split them
>> into
>> single conditions and use
>> continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge if()
>> that is quite
>> hard to read.
>>
>> 2) Yes I thought about that
>> aswell, that not all
>> constant nodes
>> are asm
>> flags, but the ARM backend does
>> this, so
>> either they
>> have a bug
>> there or
>> it's safe?
>> I prefer if you could move the
>> big condition
>> inside the
>> for()
>> into the
>> top of the loop, it makes the
>> for() quite
>> unreadable.
>> And adjust
>> there
>> the i variable as needed for each
>> suboperand
>> as you
>> mentioned
>> before.
>>
>> 4) Why do you say they handle
>> everything in the
>> SelectInlineAsm
>> method?
>> As far as i can see they only
>> handle one
>> specific case,
>> otherwise they
>> return NULL and let the default
>> behaviour do
>> the work.
>>
>> 6) Yes glues are a bit tricky,
>> dont worry
>> about them, I
>> will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you run the
>> regression
>> tests, enable
>> locally the
>> machine verifier to catch any
>> additional
>> errors. To
>> enable it
>> unconditionally search for the
>> command object
>> (i think it's
>> declared in
>> codegen/passes.cpp as a cl::opt)
>> and turn it on by
>> default. When
>> you're
>> doing your own tests you can
>> enable it passing
>> -verify-machineisntrs or
>> something like that to llc.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-17 12:13:55
|
Hi Borja.
It should work for all other constraints as well. I don't like idea of
fixing clang too. So we can just update getRegForInlineAsmConstraint method.
I'd tried to compile avr-libc. Currently clang doesn't want to compile
everything with -O0. While configure script tests everything with -O0.
Do you have any ideas how to fix -O0 ?
-Stepan.
Borja Ferrer wrote:
> Also, I guess doing only this for r is not enough, you can use
> multibytes with other register constraints.
>
>
> 2013/7/16 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>>
>
> What are you suggesting to do in clang?
>
>
> 2013/7/16 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>>
>
> Back to multibyte referenced.
>
> I've attached patch with this feature + unit-test. There is some
> question though.
> Currently, .c code:
> int a;
> void f() {
> asm("instr %A0 %B0": : "r"(a));
> }
> is transformed into IR below:
> define void @f(i16 %a) {
> entry:
> call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
> ret void
> }
> To prevent the crash I had to fix getRegForInlineAsmConstraint,
> I set it to return DREGS when it see i16 type or bigger. Though,
> may be you want to fix clang and replace "r" constraint with
> another one.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Just remembered after hitting resend that this has to be
> done always,
> similarly like in SelectAddr(). However in this case it can
> be done
> unconditionally since imm is guaranteed to fit in 6bits, so
> even if it
> comes in a constant node of type i64 it's safe to truncate
> it to i8.
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().____getZExtValue() < 64;
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed because
> i found it
> redundant:
> > + if (ImmNode->getValueType(0) != MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > +
> ImmNode->getAPIntValue().____getZExtValue(), MVT::i8);
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp", and got
> the crash. I
> don't remember what crash says exactly. Currently, I'm
> not able to
> compile anything. What I had found out is that Disp (aka
> displacement) should be i8 only. Our back-end just
> don't expect
> other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Thanks!
>
> I've been cleaning a bit the code, and fixed a
> possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().____getZExtValue() < 64;
>
> Since ImmNode comes from a dyncast, if it's NULL
> the second line
> would crash.
>
>
>
> There's a piece of code i initially removed because
> i found it
> redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> +
> ImmNode->getAPIntValue().____getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you explain why
> this happens
> and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>>
>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better, atleast fixing
> patches
> iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests with the
> machine
> verifier enabled
> then
> feel free to commit it. I'll take a look
> at the glue
> operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> Hello Borja.
> Seems I've made things much simpler.
> Can't
> understand why I
> didn't
> it in beginning.
>
> I moved everything into
> SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at all. Since it
> implemented in
> hook caller.
> #2: This problem disappeared, see #1.
> #4: The same.
> #6: Still not sure about glues. I
> just tried to
> keep all
> chains in
> proper order. But you rather look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you run the
> regression tests,
> enable locally the
> > machine verifier to catch any
> additional errors. To
> enable it
> > unconditionally search for the
> command object
> (i think it's
> declared in
> > codegen/passes.cpp as a cl::opt)
> and turn it on by
> default. When
> you're
> > doing your own tests you can
> enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really big
> conditions xD.
> You could
> split them
> into
> single conditions and use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge if()
> that is quite
> hard to read.
>
> 2) Yes I thought about that
> aswell, that not all
> constant nodes
> are asm
> flags, but the ARM backend does
> this, so
> either they
> have a bug
> there or
> it's safe?
> I prefer if you could move the
> big condition
> inside the
> for()
> into the
> top of the loop, it makes the
> for() quite
> unreadable.
> And adjust
> there
> the i variable as needed for each
> suboperand
> as you
> mentioned
> before.
>
> 4) Why do you say they handle
> everything in the
> SelectInlineAsm
> method?
> As far as i can see they only
> handle one
> specific case,
> otherwise they
> return NULL and let the default
> behaviour do
> the work.
>
> 6) Yes glues are a bit tricky,
> dont worry
> about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you run the
> regression
> tests, enable
> locally the
> machine verifier to catch any
> additional
> errors. To
> enable it
> unconditionally search for the
> command object
> (i think it's
> declared in
> codegen/passes.cpp as a cl::opt)
> and turn it on by
> default. When
> you're
> doing your own tests you can
> enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-16 13:18:40
|
Also, I guess doing only this for r is not enough, you can use multibytes
with other register constraints.
2013/7/16 Borja Ferrer <bor...@gm...>
> What are you suggesting to do in clang?
>
>
> 2013/7/16 Stepan Dyatkovskiy <stp...@na...>
>
>> Back to multibyte referenced.
>>
>> I've attached patch with this feature + unit-test. There is some question
>> though.
>> Currently, .c code:
>> int a;
>> void f() {
>> asm("instr %A0 %B0": : "r"(a));
>> }
>> is transformed into IR below:
>> define void @f(i16 %a) {
>> entry:
>> call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
>> ret void
>> }
>> To prevent the crash I had to fix getRegForInlineAsmConstraint, I set it
>> to return DREGS when it see i16 type or bigger. Though, may be you want to
>> fix clang and replace "r" constraint with another one.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>>> Just remembered after hitting resend that this has to be done always,
>>> similarly like in SelectAddr(). However in this case it can be done
>>> unconditionally since imm is guaranteed to fit in 6bits, so even if it
>>> comes in a constant node of type i64 it's safe to truncate it to i8.
>>>
>>>
>>> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:
>>> stp...@na...>>
>>>
>>> Hi Borja,
>>>
>>>
>>> > + CanHandleRegImmOpt &= ImmNode != 0;
>>> > + CanHandleRegImmOpt &=
>>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>>
>>> You absolutely right here. Thanks.
>>>
>>>
>>> > There's a piece of code i initially removed because i found it
>>> redundant:
>>> > + if (ImmNode->getValueType(0) != MVT::i8)
>>> > + {
>>> > + Disp = CurDAG->getTargetConstant(
>>> > + ImmNode->getAPIntValue().__**getZExtValue(),
>>> MVT::i8);
>>> > + }
>>> > + else
>>> > + {
>>> > + Disp = ImmOp;
>>> > + }
>>>
>>> Initially did the same: just "Disp = ImmOp", and got the crash. I
>>> don't remember what crash says exactly. Currently, I'm not able to
>>> compile anything. What I had found out is that Disp (aka
>>> displacement) should be i8 only. Our back-end just don't expect
>>> other types of displacements.
>>>
>>> -Stepan.
>>>
>>> Borja Ferrer wrote:
>>>
>>> Thanks!
>>>
>>> I've been cleaning a bit the code, and fixed a possible crash
>>> here:
>>>
>>> + CanHandleRegImmOpt &= ImmNode != 0;
>>> + CanHandleRegImmOpt &=
>>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>>
>>> Since ImmNode comes from a dyncast, if it's NULL the second line
>>> would crash.
>>>
>>>
>>>
>>> There's a piece of code i initially removed because i found it
>>> redundant:
>>> + if (ImmNode->getValueType(0) != MVT::i8)
>>> + {
>>> + Disp = CurDAG->getTargetConstant(
>>> + ImmNode->getAPIntValue().__**getZExtValue(),
>>> MVT::i8);
>>> + }
>>> + else
>>> + {
>>> + Disp = ImmOp;
>>> + }
>>>
>>> but without im getting crashes. Can you explain why this happens
>>> and is needed?
>>>
>>>
>>>
>>> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
>>> <mailto:stp...@na...> <mailto:stp...@na...
>>> <mailto:stp...@na...>>>
>>>
>>>
>>> r269
>>> -Stepan
>>> Borja Ferrer wrote:
>>>
>>> Ohhh this is a lot better, atleast fixing patches
>>> iteratively
>>> brought us
>>> to a nice solution :)
>>>
>>> If this patch passes all tests with the machine
>>> verifier enabled
>>> then
>>> feel free to commit it. I'll take a look at the glue
>>> operands
>>> after this.
>>>
>>>
>>> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
>>> <mailto:stp...@na...>
>>> <mailto:stp...@na... <mailto:stp...@na...>>
>>> <mailto:stp...@na... <mailto:stp...@na...>
>>>
>>> <mailto:stp...@na... <mailto:stp...@na...>>>>
>>>
>>>
>>> Hello Borja.
>>> Seems I've made things much simpler. Can't
>>> understand why I
>>> didn't
>>> it in beginning.
>>>
>>> I moved everything into
>>> SelectInlineAsmMemoryOperand hook.
>>> #1: We don't need 'for' at all. Since it
>>> implemented in
>>> hook caller.
>>> #2: This problem disappeared, see #1.
>>> #4: The same.
>>> #6: Still not sure about glues. I just tried to
>>> keep all
>>> chains in
>>> proper order. But you rather look at new
>>> SelectInlineAsmMemoryOperand.
>>>
>>>
>>> > One last thing, when you run the regression
>>> tests,
>>> enable locally the
>>> > machine verifier to catch any additional
>>> errors. To
>>> enable it
>>> > unconditionally search for the command object
>>> (i think it's
>>> declared in
>>> > codegen/passes.cpp as a cl::opt) and turn it on
>>> by
>>> default. When
>>> you're
>>> > doing your own tests you can enable it passing
>>> -verify-machineisntrs or
>>> > something like that to llc.
>>> OK.
>>>
>>> -Stepan.
>>>
>>>
>>> Borja Ferrer wrote:
>>>
>>> Hello Stepan,
>>>
>>> 1) Wow, those are really big conditions xD.
>>> You could
>>> split them
>>> into
>>> single conditions and use continues:
>>> if (cond 1) continue;
>>> if (cond 2) continue;
>>> etc...
>>>
>>> instead of having a huge if() that is quite
>>> hard to read.
>>>
>>> 2) Yes I thought about that aswell, that not
>>> all
>>> constant nodes
>>> are asm
>>> flags, but the ARM backend does this, so
>>> either they
>>> have a bug
>>> there or
>>> it's safe?
>>> I prefer if you could move the big condition
>>> inside the
>>> for()
>>> into the
>>> top of the loop, it makes the for() quite
>>> unreadable.
>>> And adjust
>>> there
>>> the i variable as needed for each suboperand
>>> as you
>>> mentioned
>>> before.
>>>
>>> 4) Why do you say they handle everything in the
>>> SelectInlineAsm
>>> method?
>>> As far as i can see they only handle one
>>> specific case,
>>> otherwise they
>>> return NULL and let the default behaviour do
>>> the work.
>>>
>>> 6) Yes glues are a bit tricky, dont worry
>>> about them, I
>>> will fix
>>> them
>>> when this gets commited.
>>>
>>> One last thing, when you run the regression
>>> tests, enable
>>> locally the
>>> machine verifier to catch any additional
>>> errors. To
>>> enable it
>>> unconditionally search for the command object
>>> (i think it's
>>> declared in
>>> codegen/passes.cpp as a cl::opt) and turn it
>>> on by
>>> default. When
>>> you're
>>> doing your own tests you can enable it passing
>>> -verify-machineisntrs or
>>> something like that to llc.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-16 13:09:32
|
What are you suggesting to do in clang?
2013/7/16 Stepan Dyatkovskiy <stp...@na...>
> Back to multibyte referenced.
>
> I've attached patch with this feature + unit-test. There is some question
> though.
> Currently, .c code:
> int a;
> void f() {
> asm("instr %A0 %B0": : "r"(a));
> }
> is transformed into IR below:
> define void @f(i16 %a) {
> entry:
> call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
> ret void
> }
> To prevent the crash I had to fix getRegForInlineAsmConstraint, I set it
> to return DREGS when it see i16 type or bigger. Though, may be you want to
> fix clang and replace "r" constraint with another one.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
>> Just remembered after hitting resend that this has to be done always,
>> similarly like in SelectAddr(). However in this case it can be done
>> unconditionally since imm is guaranteed to fit in 6bits, so even if it
>> comes in a constant node of type i64 it's safe to truncate it to i8.
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
>> >>
>>
>> Hi Borja,
>>
>>
>> > + CanHandleRegImmOpt &= ImmNode != 0;
>> > + CanHandleRegImmOpt &=
>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>
>> You absolutely right here. Thanks.
>>
>>
>> > There's a piece of code i initially removed because i found it
>> redundant:
>> > + if (ImmNode->getValueType(0) != MVT::i8)
>> > + {
>> > + Disp = CurDAG->getTargetConstant(
>> > + ImmNode->getAPIntValue().__**getZExtValue(),
>> MVT::i8);
>> > + }
>> > + else
>> > + {
>> > + Disp = ImmOp;
>> > + }
>>
>> Initially did the same: just "Disp = ImmOp", and got the crash. I
>> don't remember what crash says exactly. Currently, I'm not able to
>> compile anything. What I had found out is that Disp (aka
>> displacement) should be i8 only. Our back-end just don't expect
>> other types of displacements.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>> Thanks!
>>
>> I've been cleaning a bit the code, and fixed a possible crash
>> here:
>>
>> + CanHandleRegImmOpt &= ImmNode != 0;
>> + CanHandleRegImmOpt &=
>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>
>> Since ImmNode comes from a dyncast, if it's NULL the second line
>> would crash.
>>
>>
>>
>> There's a piece of code i initially removed because i found it
>> redundant:
>> + if (ImmNode->getValueType(0) != MVT::i8)
>> + {
>> + Disp = CurDAG->getTargetConstant(
>> + ImmNode->getAPIntValue().__**getZExtValue(),
>> MVT::i8);
>> + }
>> + else
>> + {
>> + Disp = ImmOp;
>> + }
>>
>> but without im getting crashes. Can you explain why this happens
>> and is needed?
>>
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>> <mailto:stp...@na...>>>
>>
>>
>> r269
>> -Stepan
>> Borja Ferrer wrote:
>>
>> Ohhh this is a lot better, atleast fixing patches
>> iteratively
>> brought us
>> to a nice solution :)
>>
>> If this patch passes all tests with the machine
>> verifier enabled
>> then
>> feel free to commit it. I'll take a look at the glue
>> operands
>> after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...>>
>> <mailto:stp...@na... <mailto:stp...@na...>
>>
>> <mailto:stp...@na... <mailto:stp...@na...>>>>
>>
>>
>> Hello Borja.
>> Seems I've made things much simpler. Can't
>> understand why I
>> didn't
>> it in beginning.
>>
>> I moved everything into
>> SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at all. Since it
>> implemented in
>> hook caller.
>> #2: This problem disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about glues. I just tried to
>> keep all
>> chains in
>> proper order. But you rather look at new
>> SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you run the regression
>> tests,
>> enable locally the
>> > machine verifier to catch any additional errors.
>> To
>> enable it
>> > unconditionally search for the command object
>> (i think it's
>> declared in
>> > codegen/passes.cpp as a cl::opt) and turn it on
>> by
>> default. When
>> you're
>> > doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really big conditions xD.
>> You could
>> split them
>> into
>> single conditions and use continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge if() that is quite
>> hard to read.
>>
>> 2) Yes I thought about that aswell, that not all
>> constant nodes
>> are asm
>> flags, but the ARM backend does this, so
>> either they
>> have a bug
>> there or
>> it's safe?
>> I prefer if you could move the big condition
>> inside the
>> for()
>> into the
>> top of the loop, it makes the for() quite
>> unreadable.
>> And adjust
>> there
>> the i variable as needed for each suboperand
>> as you
>> mentioned
>> before.
>>
>> 4) Why do you say they handle everything in the
>> SelectInlineAsm
>> method?
>> As far as i can see they only handle one
>> specific case,
>> otherwise they
>> return NULL and let the default behaviour do
>> the work.
>>
>> 6) Yes glues are a bit tricky, dont worry
>> about them, I
>> will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you run the regression
>> tests, enable
>> locally the
>> machine verifier to catch any additional
>> errors. To
>> enable it
>> unconditionally search for the command object
>> (i think it's
>> declared in
>> codegen/passes.cpp as a cl::opt) and turn it on
>> by
>> default. When
>> you're
>> doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> something like that to llc.
>>
>>
>>
>>
>>
>>
>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-16 11:34:55
|
Back to multibyte referenced.
I've attached patch with this feature + unit-test. There is some
question though.
Currently, .c code:
int a;
void f() {
asm("instr %A0 %B0": : "r"(a));
}
is transformed into IR below:
define void @f(i16 %a) {
entry:
call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
ret void
}
To prevent the crash I had to fix getRegForInlineAsmConstraint, I set it
to return DREGS when it see i16 type or bigger. Though, may be you want
to fix clang and replace "r" constraint with another one.
-Stepan.
Borja Ferrer wrote:
> Just remembered after hitting resend that this has to be done always,
> similarly like in SelectAddr(). However in this case it can be done
> unconditionally since imm is guaranteed to fit in 6bits, so even if it
> comes in a constant node of type i64 it's safe to truncate it to i8.
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>>
>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().__getZExtValue() < 64;
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed because i found it
> redundant:
> > + if (ImmNode->getValueType(0) != MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > + ImmNode->getAPIntValue().__getZExtValue(), MVT::i8);
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp", and got the crash. I
> don't remember what crash says exactly. Currently, I'm not able to
> compile anything. What I had found out is that Disp (aka
> displacement) should be i8 only. Our back-end just don't expect
> other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Thanks!
>
> I've been cleaning a bit the code, and fixed a possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().__getZExtValue() < 64;
>
> Since ImmNode comes from a dyncast, if it's NULL the second line
> would crash.
>
>
>
> There's a piece of code i initially removed because i found it
> redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> + ImmNode->getAPIntValue().__getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you explain why this happens
> and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better, atleast fixing patches
> iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests with the machine
> verifier enabled
> then
> feel free to commit it. I'll take a look at the glue
> operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na... <mailto:stp...@na...>
>
> <mailto:stp...@na... <mailto:stp...@na...>>>>
>
>
> Hello Borja.
> Seems I've made things much simpler. Can't
> understand why I
> didn't
> it in beginning.
>
> I moved everything into
> SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at all. Since it
> implemented in
> hook caller.
> #2: This problem disappeared, see #1.
> #4: The same.
> #6: Still not sure about glues. I just tried to
> keep all
> chains in
> proper order. But you rather look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you run the regression tests,
> enable locally the
> > machine verifier to catch any additional errors. To
> enable it
> > unconditionally search for the command object
> (i think it's
> declared in
> > codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> > doing your own tests you can enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really big conditions xD.
> You could
> split them
> into
> single conditions and use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge if() that is quite
> hard to read.
>
> 2) Yes I thought about that aswell, that not all
> constant nodes
> are asm
> flags, but the ARM backend does this, so
> either they
> have a bug
> there or
> it's safe?
> I prefer if you could move the big condition
> inside the
> for()
> into the
> top of the loop, it makes the for() quite
> unreadable.
> And adjust
> there
> the i variable as needed for each suboperand
> as you
> mentioned
> before.
>
> 4) Why do you say they handle everything in the
> SelectInlineAsm
> method?
> As far as i can see they only handle one
> specific case,
> otherwise they
> return NULL and let the default behaviour do
> the work.
>
> 6) Yes glues are a bit tricky, dont worry
> about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you run the regression
> tests, enable
> locally the
> machine verifier to catch any additional
> errors. To
> enable it
> unconditionally search for the command object
> (i think it's
> declared in
> codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> doing your own tests you can enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-15 18:34:15
|
Just remembered after hitting resend that this has to be done always,
similarly like in SelectAddr(). However in this case it can be done
unconditionally since imm is guaranteed to fit in 6bits, so even if it
comes in a constant node of type i64 it's safe to truncate it to i8.
2013/7/15 Stepan Dyatkovskiy <stp...@na...>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &= ImmNode->getAPIntValue().**getZExtValue() <
> 64;
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed because i found it redundant:
> > + if (ImmNode->getValueType(0) != MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > + ImmNode->getAPIntValue().**getZExtValue(), MVT::i8);
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp", and got the crash. I don't
> remember what crash says exactly. Currently, I'm not able to compile
> anything. What I had found out is that Disp (aka displacement) should be i8
> only. Our back-end just don't expect other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
>> Thanks!
>>
>> I've been cleaning a bit the code, and fixed a possible crash here:
>>
>> + CanHandleRegImmOpt &= ImmNode != 0;
>> + CanHandleRegImmOpt &= ImmNode->getAPIntValue().**getZExtValue() <
>> 64;
>>
>> Since ImmNode comes from a dyncast, if it's NULL the second line would
>> crash.
>>
>>
>>
>> There's a piece of code i initially removed because i found it redundant:
>> + if (ImmNode->getValueType(0) != MVT::i8)
>> + {
>> + Disp = CurDAG->getTargetConstant(
>> + ImmNode->getAPIntValue().**getZExtValue(), MVT::i8);
>> + }
>> + else
>> + {
>> + Disp = ImmOp;
>> + }
>>
>> but without im getting crashes. Can you explain why this happens and is
>> needed?
>>
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
>> >>
>>
>>
>> r269
>> -Stepan
>> Borja Ferrer wrote:
>>
>> Ohhh this is a lot better, atleast fixing patches iteratively
>> brought us
>> to a nice solution :)
>>
>> If this patch passes all tests with the machine verifier enabled
>> then
>> feel free to commit it. I'll take a look at the glue operands
>> after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>>
>> <mailto:stp...@na...>>>
>>
>>
>> Hello Borja.
>> Seems I've made things much simpler. Can't understand why I
>> didn't
>> it in beginning.
>>
>> I moved everything into SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at all. Since it implemented in
>> hook caller.
>> #2: This problem disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about glues. I just tried to keep all
>> chains in
>> proper order. But you rather look at new
>> SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you run the regression tests,
>> enable locally the
>> > machine verifier to catch any additional errors. To
>> enable it
>> > unconditionally search for the command object (i think
>> it's
>> declared in
>> > codegen/passes.cpp as a cl::opt) and turn it on by
>> default. When
>> you're
>> > doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really big conditions xD. You could
>> split them
>> into
>> single conditions and use continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge if() that is quite hard to read.
>>
>> 2) Yes I thought about that aswell, that not all
>> constant nodes
>> are asm
>> flags, but the ARM backend does this, so either they
>> have a bug
>> there or
>> it's safe?
>> I prefer if you could move the big condition inside the
>> for()
>> into the
>> top of the loop, it makes the for() quite unreadable.
>> And adjust
>> there
>> the i variable as needed for each suboperand as you
>> mentioned
>> before.
>>
>> 4) Why do you say they handle everything in the
>> SelectInlineAsm
>> method?
>> As far as i can see they only handle one specific case,
>> otherwise they
>> return NULL and let the default behaviour do the work.
>>
>> 6) Yes glues are a bit tricky, dont worry about them, I
>> will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you run the regression tests, enable
>> locally the
>> machine verifier to catch any additional errors. To
>> enable it
>> unconditionally search for the command object (i think
>> it's
>> declared in
>> codegen/passes.cpp as a cl::opt) and turn it on by
>> default. When
>> you're
>> doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> something like that to llc.
>>
>>
>>
>>
>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-15 17:30:39
|
Hi Borja,
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &= ImmNode->getAPIntValue().getZExtValue() < 64;
You absolutely right here. Thanks.
> There's a piece of code i initially removed because i found it redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> + ImmNode->getAPIntValue().getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
Initially did the same: just "Disp = ImmOp", and got the crash. I don't
remember what crash says exactly. Currently, I'm not able to compile
anything. What I had found out is that Disp (aka displacement) should be
i8 only. Our back-end just don't expect other types of displacements.
-Stepan.
Borja Ferrer wrote:
> Thanks!
>
> I've been cleaning a bit the code, and fixed a possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &= ImmNode->getAPIntValue().getZExtValue() < 64;
>
> Since ImmNode comes from a dyncast, if it's NULL the second line would crash.
>
>
>
> There's a piece of code i initially removed because i found it redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> + ImmNode->getAPIntValue().getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you explain why this happens and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better, atleast fixing patches iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests with the machine verifier enabled
> then
> feel free to commit it. I'll take a look at the glue operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
>
> Hello Borja.
> Seems I've made things much simpler. Can't understand why I
> didn't
> it in beginning.
>
> I moved everything into SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at all. Since it implemented in
> hook caller.
> #2: This problem disappeared, see #1.
> #4: The same.
> #6: Still not sure about glues. I just tried to keep all
> chains in
> proper order. But you rather look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you run the regression tests,
> enable locally the
> > machine verifier to catch any additional errors. To
> enable it
> > unconditionally search for the command object (i think it's
> declared in
> > codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> > doing your own tests you can enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really big conditions xD. You could
> split them
> into
> single conditions and use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge if() that is quite hard to read.
>
> 2) Yes I thought about that aswell, that not all
> constant nodes
> are asm
> flags, but the ARM backend does this, so either they
> have a bug
> there or
> it's safe?
> I prefer if you could move the big condition inside the
> for()
> into the
> top of the loop, it makes the for() quite unreadable.
> And adjust
> there
> the i variable as needed for each suboperand as you
> mentioned
> before.
>
> 4) Why do you say they handle everything in the
> SelectInlineAsm
> method?
> As far as i can see they only handle one specific case,
> otherwise they
> return NULL and let the default behaviour do the work.
>
> 6) Yes glues are a bit tricky, dont worry about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you run the regression tests, enable
> locally the
> machine verifier to catch any additional errors. To
> enable it
> unconditionally search for the command object (i think it's
> declared in
> codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> doing your own tests you can enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-15 16:52:47
|
Thanks!
I've been cleaning a bit the code, and fixed a possible crash here:
+ CanHandleRegImmOpt &= ImmNode != 0;+ CanHandleRegImmOpt &=
ImmNode->getAPIntValue().getZExtValue() < 64;
Since ImmNode comes from a dyncast, if it's NULL the second line would crash.
There's a piece of code i initially removed because i found it redundant:
+ if (ImmNode->getValueType(0) != MVT::i8)+ {+ Disp =
CurDAG->getTargetConstant(+
ImmNode->getAPIntValue().getZExtValue(), MVT::i8);+ }+ else+
{+ Disp = ImmOp;+ }
but without im getting crashes. Can you explain why this happens and is needed?
2013/7/15 Stepan Dyatkovskiy <stp...@na...>
> r269
> -Stepan
> Borja Ferrer wrote:
>
>> Ohhh this is a lot better, atleast fixing patches iteratively brought us
>> to a nice solution :)
>>
>> If this patch passes all tests with the machine verifier enabled then
>> feel free to commit it. I'll take a look at the glue operands after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
>> >>
>>
>>
>> Hello Borja.
>> Seems I've made things much simpler. Can't understand why I didn't
>> it in beginning.
>>
>> I moved everything into SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at all. Since it implemented in hook caller.
>> #2: This problem disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about glues. I just tried to keep all chains in
>> proper order. But you rather look at new SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you run the regression tests, enable locally
>> the
>> > machine verifier to catch any additional errors. To enable it
>> > unconditionally search for the command object (i think it's
>> declared in
>> > codegen/passes.cpp as a cl::opt) and turn it on by default. When
>> you're
>> > doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really big conditions xD. You could split them
>> into
>> single conditions and use continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge if() that is quite hard to read.
>>
>> 2) Yes I thought about that aswell, that not all constant nodes
>> are asm
>> flags, but the ARM backend does this, so either they have a bug
>> there or
>> it's safe?
>> I prefer if you could move the big condition inside the for()
>> into the
>> top of the loop, it makes the for() quite unreadable. And adjust
>> there
>> the i variable as needed for each suboperand as you mentioned
>> before.
>>
>> 4) Why do you say they handle everything in the SelectInlineAsm
>> method?
>> As far as i can see they only handle one specific case,
>> otherwise they
>> return NULL and let the default behaviour do the work.
>>
>> 6) Yes glues are a bit tricky, dont worry about them, I will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you run the regression tests, enable
>> locally the
>> machine verifier to catch any additional errors. To enable it
>> unconditionally search for the command object (i think it's
>> declared in
>> codegen/passes.cpp as a cl::opt) and turn it on by default. When
>> you're
>> doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> something like that to llc.
>>
>>
>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-15 12:04:53
|
r269 -Stepan Borja Ferrer wrote: > Ohhh this is a lot better, atleast fixing patches iteratively brought us > to a nice solution :) > > If this patch passes all tests with the machine verifier enabled then > feel free to commit it. I'll take a look at the glue operands after this. > > > 2013/7/12 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>> > > Hello Borja. > Seems I've made things much simpler. Can't understand why I didn't > it in beginning. > > I moved everything into SelectInlineAsmMemoryOperand hook. > #1: We don't need 'for' at all. Since it implemented in hook caller. > #2: This problem disappeared, see #1. > #4: The same. > #6: Still not sure about glues. I just tried to keep all chains in > proper order. But you rather look at new SelectInlineAsmMemoryOperand. > > > > One last thing, when you run the regression tests, enable locally the > > machine verifier to catch any additional errors. To enable it > > unconditionally search for the command object (i think it's > declared in > > codegen/passes.cpp as a cl::opt) and turn it on by default. When > you're > > doing your own tests you can enable it passing > -verify-machineisntrs or > > something like that to llc. > OK. > > -Stepan. > > > Borja Ferrer wrote: > > Hello Stepan, > > 1) Wow, those are really big conditions xD. You could split them > into > single conditions and use continues: > if (cond 1) continue; > if (cond 2) continue; > etc... > > instead of having a huge if() that is quite hard to read. > > 2) Yes I thought about that aswell, that not all constant nodes > are asm > flags, but the ARM backend does this, so either they have a bug > there or > it's safe? > I prefer if you could move the big condition inside the for() > into the > top of the loop, it makes the for() quite unreadable. And adjust > there > the i variable as needed for each suboperand as you mentioned > before. > > 4) Why do you say they handle everything in the SelectInlineAsm > method? > As far as i can see they only handle one specific case, > otherwise they > return NULL and let the default behaviour do the work. > > 6) Yes glues are a bit tricky, dont worry about them, I will fix > them > when this gets commited. > > One last thing, when you run the regression tests, enable > locally the > machine verifier to catch any additional errors. To enable it > unconditionally search for the command object (i think it's > declared in > codegen/passes.cpp as a cl::opt) and turn it on by default. When > you're > doing your own tests you can enable it passing > -verify-machineisntrs or > something like that to llc. > > > |
|
From: Borja F. <bor...@gm...> - 2013-07-12 15:00:38
|
Ohhh this is a lot better, atleast fixing patches iteratively brought us to a nice solution :) If this patch passes all tests with the machine verifier enabled then feel free to commit it. I'll take a look at the glue operands after this. 2013/7/12 Stepan Dyatkovskiy <stp...@na...> > Hello Borja. > Seems I've made things much simpler. Can't understand why I didn't it in > beginning. > > I moved everything into SelectInlineAsmMemoryOperand hook. > #1: We don't need 'for' at all. Since it implemented in hook caller. > #2: This problem disappeared, see #1. > #4: The same. > #6: Still not sure about glues. I just tried to keep all chains in proper > order. But you rather look at new SelectInlineAsmMemoryOperand. > > > > One last thing, when you run the regression tests, enable locally the > > machine verifier to catch any additional errors. To enable it > > unconditionally search for the command object (i think it's declared in > > codegen/passes.cpp as a cl::opt) and turn it on by default. When you're > > doing your own tests you can enable it passing -verify-machineisntrs or > > something like that to llc. > OK. > > -Stepan. > > > Borja Ferrer wrote: > >> Hello Stepan, >> >> 1) Wow, those are really big conditions xD. You could split them into >> single conditions and use continues: >> if (cond 1) continue; >> if (cond 2) continue; >> etc... >> >> instead of having a huge if() that is quite hard to read. >> >> 2) Yes I thought about that aswell, that not all constant nodes are asm >> flags, but the ARM backend does this, so either they have a bug there or >> it's safe? >> I prefer if you could move the big condition inside the for() into the >> top of the loop, it makes the for() quite unreadable. And adjust there >> the i variable as needed for each suboperand as you mentioned before. >> >> 4) Why do you say they handle everything in the SelectInlineAsm method? >> As far as i can see they only handle one specific case, otherwise they >> return NULL and let the default behaviour do the work. >> >> 6) Yes glues are a bit tricky, dont worry about them, I will fix them >> when this gets commited. >> >> One last thing, when you run the regression tests, enable locally the >> machine verifier to catch any additional errors. To enable it >> unconditionally search for the command object (i think it's declared in >> codegen/passes.cpp as a cl::opt) and turn it on by default. When you're >> doing your own tests you can enable it passing -verify-machineisntrs or >> something like that to llc. >> >> > |
14 messages has been excluded from this view by a project administrator.
