| Submitter | Maciej Pijanka |
|---|---|
| Date | 2010-03-15 20:48:59 |
| Message ID | <20100315204858.GA3204@debshine> |
| Download | mbox | patch |
| Permalink | /patch/1057/ |
| State | Not Applicable |
| Headers | show |
Comments
On Mon, Mar 15, 2010 at 2:48 PM, Maciej Pijanka <maciej.pijanka@gmail.com>wrote: > Hello > > On my target gcc complains about undefined CONFIG_SSE2 macro and defaults > it to 0, maybe attached patch should quiet gcc a bit? > I think the preferred way to quiet this warning would be to define SSE2 correctly in src/cpu/VENDOR/CPU/Kconfig. The next best would be to set SSE2 to default to n in src/cpu/Kconfig. Thanks, Myles
On Mon, 15 Mar 2010, Myles Watson wrote: > On Mon, Mar 15, 2010 at 2:48 PM, Maciej Pijanka <maciej.pijanka@gmail.com>wrote: > > > Hello > > > > On my target gcc complains about undefined CONFIG_SSE2 macro and defaults > > it to 0, maybe attached patch should quiet gcc a bit? > > > I think the preferred way to quiet this warning would be to define SSE2 > correctly in src/cpu/VENDOR/CPU/Kconfig. The next best would be to set SSE2 > to default to n in src/cpu/Kconfig. I have such warning for P2, i checked and no my p2 cpus have SSE2, but i am not going to patch src/cpu/ just because i don't understand what impact it might have. I understand that this patch should not be needed, but then, maybe add before #define in patch to #warning "cpu don't define CONFIG_SSE2, fix it if you can" or something similar? Maciej
On 03/15/2010 06:10 PM, Maciej Pijanka wrote: > On Mon, 15 Mar 2010, Myles Watson wrote: > >> On Mon, Mar 15, 2010 at 2:48 PM, Maciej Pijanka<maciej.pijanka@gmail.com>wrote: >> >>> Hello >>> >>> On my target gcc complains about undefined CONFIG_SSE2 macro and defaults >>> it to 0, maybe attached patch should quiet gcc a bit? >>> >> I think the preferred way to quiet this warning would be to define SSE2 >> correctly in src/cpu/VENDOR/CPU/Kconfig. The next best would be to set SSE2 >> to default to n in src/cpu/Kconfig. > > I have such warning for P2, i checked and no my p2 cpus have SSE2, but i am not going to patch > src/cpu/ just because i don't understand what impact it might have. > > I understand that this patch should not be needed, but then, maybe add before #define in patch > to #warning "cpu don't define CONFIG_SSE2, fix it if you can" or something similar? > Yes I agree p2's do not support SSE2, so one would think you would not even get this warning.....
> >>> On my target gcc complains about undefined CONFIG_SSE2 macro and > defaults > >>> it to 0, maybe attached patch should quiet gcc a bit? > >>> > >> I think the preferred way to quiet this warning would be to define SSE2 > >> correctly in src/cpu/VENDOR/CPU/Kconfig. The next best would be to set > SSE2 > >> to default to n in src/cpu/Kconfig. > > > > I have such warning for P2, i checked and no my p2 cpus have SSE2, but i > am not going to patch > > src/cpu/ just because i don't understand what impact it might have. Then silencing the warning is probably not a good idea :) You can grep for CONFIG_SSE2 to see where the variable is checked. The idea is that you can speed up execution if you have SSE2 instructions available, but we don't want to break things for those that don't have them. The warning is there to suggest that you put in the correct value. > > I understand that this patch should not be needed, but then, maybe add > before #define in patch > > to #warning "cpu don't define CONFIG_SSE2, fix it if you can" or > something similar? > > > Yes I agree p2's do not support SSE2, so one would think you would not > even get this warning..... You get the warning because no one has told Coreboot that p2s don't have SSE2. That should be done in the Kconfig file for the processor. Thanks, Myles
> The idea is that you can > speed up execution if you have SSE2 instructions available, but we don't > want to break things for those that don't have them. The warning is there > to suggest that you put in the correct value > I would welcome a patch where someone ran ./util/abuild/abuild -C -a grep -L "SSE " coreboot-builds/*/config.h grep -L "SSE2 " coreboot-builds/*/config.h Then added the correct values to the src/cpu/*/*/Kconfig files. It would take some time with wikipedia. On the other hand, since you know your CPU doesn't support SSE2, you could just mention your CPU by name, and I'll add the correct value for SSE2 to the correct file. Thanks, Myles
On Mon, 15 Mar 2010, Myles Watson wrote: > > >>> On my target gcc complains about undefined CONFIG_SSE2 macro and > > defaults > > >>> it to 0, maybe attached patch should quiet gcc a bit? > > >>> > > >> I think the preferred way to quiet this warning would be to define SSE2 > > >> correctly in src/cpu/VENDOR/CPU/Kconfig. The next best would be to set > > SSE2 > > >> to default to n in src/cpu/Kconfig. > > > > > > I have such warning for P2, i checked and no my p2 cpus have SSE2, but i > > am not going to patch > > > src/cpu/ just because i don't understand what impact it might have. > Then silencing the warning is probably not a good idea :) You can grep for > CONFIG_SSE2 to see where the variable is checked. The idea is that you can > speed up execution if you have SSE2 instructions available, but we don't > want to break things for those that don't have them. The warning is there > to suggest that you put in the correct value. but gcc defaults to same value as this patch would do, so only advantage or disadvantage might be in better more descriptive comment or not. Currently plain warning about undefined macro don't attract too much attention. > > > > I understand that this patch should not be needed, but then, maybe add > > before #define in patch > > > to #warning "cpu don't define CONFIG_SSE2, fix it if you can" or > > something similar? > > > > > Yes I agree p2's do not support SSE2, so one would think you would not > > even get this warning..... > You get the warning because no one has told Coreboot that p2s don't have > SSE2. That should be done in the Kconfig file for the processor. But it should be safe to set default config SSE2 to n, so in worst case person will get degraded performance, or better is add patch which issues correct warning message not just info about missing macro, and defines to sensible default after issuing warnining (so to 0 as gcc do otherwise anyway) > > Thanks, > Myles >
Patch
Index: src/lib/ramtest.c =================================================================== --- src/lib/ramtest.c (revision 5213) +++ src/lib/ramtest.c (working copy) @@ -1,3 +1,9 @@ + +/* make compiler quiet about undefined macro */ +#ifndef CONFIG_SSE2 +#define CONFIG_SSE2 0 +#endif + static void write_phys(unsigned long addr, unsigned long value) { // Assembler in lib/ is very ugly. But we properly guarded