Re: [PATCH 0/6] PowerPC Dense Math prelimary support (-mcpu=future)
On Tue, Jan 31, 2023 at 10:31:03PM -0500, Michael Meissner wrote: > Ok, I tracked down the source of the bug. The CCP pass is depending on the > precision field. Unfortunately in tree-core.h, the precision is a 10 integer > bit field, so 1,024 will become 0. > > Having a 0 precision meant that the hwint function for sign extending a value > would generate: > > (HOST_WIDE_INT)(((unsigned HOST_WIDE_INT)value << 64) >> 64) > > which is undefined behavior in C and C++. On the x86_64 doing the shift left > and then right gives you the initial value (which was -1), while on the > PowerPC > it always gives you 0. The CCP code was assuming if it wasn't -1, that it was > an integer, but the TDO type is opaque, not integer. Variable 64-bit shifts on x86 mask the shift amount to 6 bits, while on PowerPC it is masked to 7 bits. It sounds like that is what you hit, with some -O0 build perhaps. But either way UB is UB, the program has no meaning, any output is correct, no output is correct as well :-) Nasal demons and all that. bootstrap-ubsan should have found this? Segher
Re: [PATCH 0/6] PowerPC Dense Math prelimary support (-mcpu=future)
On Sun, Jan 29, 2023 at 09:52:38PM -0500, Michael Meissner wrote: > On Sat, Jan 28, 2023 at 02:29:04AM -0500, Michael Meissner wrote: > > On Fri, Jan 27, 2023 at 01:59:00PM -0600, Segher Boessenkool wrote: > > > > There is one bug that I noticed. When you use the full DMR instruction > > > > the > > > > constant copy propagation patch issues internal errors. I believe this > > > > is due > > > > to the CCP pass not handling opaque types cleanly enough, and it only > > > > shows up > > > > in larger types. I would like to get these patches committed, and then > > > > work > > > > the maintainers of the CCP to fix the problem. > > > > > > Erm. If the compiler ICEs, we can not include this code. But hopefully > > > you mean something else? > > > > I realize we can't include the code for final release. But as a temporary > > measure I was hoping we would put in the code, we could allow somebody more > > familar with ccp to debug it. Then if there were changes needed in the > > PowerPC > > back end, we could make them, once ccp was fixed. > > > > But that is a moot point, ccp no longer dies with the code, so I have > > removed > > the comment and the no tree ccp option in the next set of patches. > > Unfortunately, while it worked on my x86 as a cross compiler, when I did the > builds for real, it is a problem, so I will need to look into it. Ok, I tracked down the source of the bug. The CCP pass is depending on the precision field. Unfortunately in tree-core.h, the precision is a 10 integer bit field, so 1,024 will become 0. Having a 0 precision meant that the hwint function for sign extending a value would generate: (HOST_WIDE_INT)(((unsigned HOST_WIDE_INT)value << 64) >> 64) which is undefined behavior in C and C++. On the x86_64 doing the shift left and then right gives you the initial value (which was -1), while on the PowerPC it always gives you 0. The CCP code was assuming if it wasn't -1, that it was an integer, but the TDO type is opaque, not integer. The solution was to grow precision by 1 bit and decrease the extra bits in the placeholder entry by 1 bit. I'm testing it now. -- Michael Meissner, IBM PO Box 98, Ayer, Massachusetts, USA, 01432 email: meiss...@linux.ibm.com
Re: [PATCH 0/6] PowerPC Dense Math prelimary support (-mcpu=future)
On Sat, Jan 28, 2023 at 02:29:04AM -0500, Michael Meissner wrote: > On Fri, Jan 27, 2023 at 01:59:00PM -0600, Segher Boessenkool wrote: > > > There is one bug that I noticed. When you use the full DMR instruction > > > the > > > constant copy propagation patch issues internal errors. I believe this > > > is due > > > to the CCP pass not handling opaque types cleanly enough, and it only > > > shows up > > > in larger types. I would like to get these patches committed, and then > > > work > > > the maintainers of the CCP to fix the problem. > > > > Erm. If the compiler ICEs, we can not include this code. But hopefully > > you mean something else? > > I realize we can't include the code for final release. But as a temporary > measure I was hoping we would put in the code, we could allow somebody more > familar with ccp to debug it. Then if there were changes needed in the > PowerPC > back end, we could make them, once ccp was fixed. > > But that is a moot point, ccp no longer dies with the code, so I have removed > the comment and the no tree ccp option in the next set of patches. Unfortunately, while it worked on my x86 as a cross compiler, when I did the builds for real, it is a problem, so I will need to look into it. -- Michael Meissner, IBM PO Box 98, Ayer, Massachusetts, USA, 01432 email: meiss...@linux.ibm.com
Re: [PATCH 0/6] PowerPC Dense Math prelimary support (-mcpu=future)
On Fri, Jan 27, 2023 at 01:59:00PM -0600, Segher Boessenkool wrote: > > There is one bug that I noticed. When you use the full DMR instruction the > > constant copy propagation patch issues internal errors. I believe this is > > due > > to the CCP pass not handling opaque types cleanly enough, and it only shows > > up > > in larger types. I would like to get these patches committed, and then work > > the maintainers of the CCP to fix the problem. > > Erm. If the compiler ICEs, we can not include this code. But hopefully > you mean something else? I realize we can't include the code for final release. But as a temporary measure I was hoping we would put in the code, we could allow somebody more familar with ccp to debug it. Then if there were changes needed in the PowerPC back end, we could make them, once ccp was fixed. But that is a moot point, ccp no longer dies with the code, so I have removed the comment and the no tree ccp option in the next set of patches. -- Michael Meissner, IBM PO Box 98, Ayer, Massachusetts, USA, 01432 email: meiss...@linux.ibm.com
Re: [PATCH 0/6] PowerPC Dense Math prelimary support (-mcpu=future)
Hi! On Wed, Nov 09, 2022 at 09:43:16PM -0500, Michael Meissner wrote: > This patch is very preliminary support for a potential new feature to the > PowerPC that extends the current power10 MMA architecture. This feature may > or > may not be present in any specific future PowerPC processor. MMA is an optional facility in ISA 3.1 -- please don't say it is power10 only. > In the current MMA subsystem for Power10, there are 8 512-bit accumulator > registers. These accumulators are each tied to sets of 4 FPR registers. Four VSRs. FPRs are only 64bits. You mean this is VSRs 0..31 . > When > you issue a prime instruction, it makes sure the accumulator is a copy of the > 4 I suppose you mean the xxmtacc instruction? > FPR registers the accumulator is tied to. When you issue a deprime > instruction, it makes sure that the accumulator data content is logically > copied to the matching FPR register. And xxmfacc. Very importantly all the other rules in 7.2.1.3 "VSX Accumulators" apply as well. That should make old code work on new systems transparently. > In terms of changes, we now use the wD constraint for accumulators. If you > compile with -mcpu=power10, the wD constraint will match the equivalent FPR > register that overlaps with the accumulator. The set of *four* *VSX* registers. Of course in the end it is just a number, but :-) > If you compile with -mcpu=future, > the wD constraint will match the DMR register and not the FPR register. Constraints do not "match" anything. "Will allow" perhaps? > In general, if you only use the built-in functions, things work between the > two > systems. If you use extended asm, you will likely need to modify the code. > Going forward, hopefully if you modify your code to use the wD constraint and > %A output modifier, you can write code that switches more easily between the > two systems. You *already* are required to follow all these rules that make this painless and transparent. > There is one bug that I noticed. When you use the full DMR instruction the > constant copy propagation patch issues internal errors. I believe this is due > to the CCP pass not handling opaque types cleanly enough, and it only shows up > in larger types. I would like to get these patches committed, and then work > the maintainers of the CCP to fix the problem. Erm. If the compiler ICEs, we can not include this code. But hopefully you mean something else? Segher
[PATCH 0/6] PowerPC Dense Math prelimary support (-mcpu=future)
This patch is very preliminary support for a potential new feature to the PowerPC that extends the current power10 MMA architecture. This feature may or may not be present in any specific future PowerPC processor. In the current MMA subsystem for Power10, there are 8 512-bit accumulator registers. These accumulators are each tied to sets of 4 FPR registers. When you issue a prime instruction, it makes sure the accumulator is a copy of the 4 FPR registers the accumulator is tied to. When you issue a deprime instruction, it makes sure that the accumulator data content is logically copied to the matching FPR register. In the potential dense math system, the accumulators are moved to separate registers called dense math registers (DM registers or DMR). The DMRs are then extended to 1,024 bits and new instructions will be added to deal with all 1,024 bits of the DMRs. If you take existing MMA code, it will work as long as you don't do anything with accumulators, and you follow the rules in the ISA 3.1 documentation for using the MMA subsystem. These patches add support for the 512-bit accumulators within the dense math system, and for allocation of the 1,024-bit DMRs. At this time, no additional built-in functions will be done to support any dense math features other than doing data movement between the DMRs and the VSX registers. Before we can look at adding any new dense math support other than data movement, we need the GCC compiler to be able to allocate and use these DMRs. There are 6 patches in this patch set: 1) The first patch just adds -mcpu=future as an option to add new support. This is similar to the -mcpu=future that we did before power10 was announced. 2) The second patch enables GCC to use the load and store vector pair instructions to optimize memory copy operations in the compiler. For power10, we needed to just stay with normal vector load/stores for memory copy operations. 3) The third patch enables 512-bit accumulators store in DMRs. This patch enables the register allocation, but it does not move the existing MMA to use these registers. 4) The fourth patch switches the MMA subsystem to use 512-bit accumulators within DMRs if you use -mcpu=future. 5) The fifth patch switches the names of the MMA instructions to use the dense math equivalent name if -mcpu=future. 6) The sixth patch enables using the full 1,024-bit DMRs. Right now, all you can do with DMRs is move a VSX register to a DMR register, and to move a DMR register to a VSX register. In terms of changes, we now use the wD constraint for accumulators. If you compile with -mcpu=power10, the wD constraint will match the equivalent FPR register that overlaps with the accumulator. If you compile with -mcpu=future, the wD constraint will match the DMR register and not the FPR register. This patch also modifies the print_operand %A output modifier to print out DMR register numbers if -mcpu=future, and continue to print out the FPR register number divided by 4 for -mcpu=power10. In general, if you only use the built-in functions, things work between the two systems. If you use extended asm, you will likely need to modify the code. Going forward, hopefully if you modify your code to use the wD constraint and %A output modifier, you can write code that switches more easily between the two systems. There is one bug that I noticed. When you use the full DMR instruction the constant copy propagation patch issues internal errors. I believe this is due to the CCP pass not handling opaque types cleanly enough, and it only shows up in larger types. I would like to get these patches committed, and then work the maintainers of the CCP to fix the problem. Again, these are preliminary patches for a potential future machine. Things will likely change in terms of implementation and usage over time. -- Michael Meissner, IBM PO Box 98, Ayer, Massachusetts, USA, 01432 email: meiss...@linux.ibm.com
