Re: [RFC][nvptx] Initialize ptx regs
On Mon, Feb 21, 2022 at 2:47 PM Tom de Vries wrote:
>
> On 2/21/22 08:54, Richard Biener wrote:
> > On Sun, Feb 20, 2022 at 11:50 PM Tom de Vries via Gcc-patches
> > wrote:
> >>
> >> Hi,
> >>
> >> With nvptx target, driver version 510.47.03 and board GT 1030 I, we run
> >> into:
> >> ...
> >> FAIL: gcc.c-torture/execute/pr53465.c -O1 execution test
> >> FAIL: gcc.c-torture/execute/pr53465.c -O2 execution test
> >> FAIL: gcc.c-torture/execute/pr53465.c -O3 -g execution test
> >> ...
> >> while the test-cases pass with nvptx-none-run -O0.
> >>
> >> The problem is that the generated ptx contains a read from an uninitialized
> >> ptx register, and the driver JIT doesn't handle this well.
> >>
> >> For -O2 and -O3, we can get rid of the FAIL using --param
> >> logical-op-non-short-circuit=0. But not for -O1.
> >>
> >> At -O1, the test-case minimizes to:
> >> ...
> >> void __attribute__((noinline, noclone))
> >> foo (int y) {
> >>int c;
> >>for (int i = 0; i < y; i++)
> >> {
> >>int d = i + 1;
> >>if (i && d <= c)
> >> __builtin_abort ();
> >>c = d;
> >> }
> >> }
> >>
> >> int main () {
> >>foo (2); return 0;
> >> }
> >> ...
> >>
> >> Note that the test-case does not contain an uninitialized use. In the
> >> first
> >> iteration, i is 0 and consequently c is not read. In the second
> >> iteration, c
> >> is read, but by that time it's already initialized by 'c = d' from the
> >> first
> >> iteration.
> >>
> >> AFAICT the problem is introduced as follows: the conditional use of c in
> >> the
> >> loop body is translated into an unconditional use of c in the loop header:
> >> ...
> >># c_1 = PHI
> >> ...
> >> which forwprop1 propagates the 'c_9 = d_7' assignment into:
> >> ...
> >># c_1 = PHI
> >> ...
> >> which ends up being translated by expand into an unconditional:
> >> ...
> >> (insn 13 12 0 (set (reg/v:SI 22 [ c ])
> >> (reg/v:SI 23 [ d ])) -1
> >> (nil))
> >> ...
> >> at the start of the loop body, creating an uninitialized read of d on the
> >> path from loop entry.
> >
> > Ah, interesting case. Note that some fixup pass inserted a copy in
> > the loop header
> > before coalescing:
> >
> > ;; basic block 3, loop depth 1
> > ;;pred: 6
> > ;;2
> ># c_10 = PHI
> ># i_11 = PHI
> >c_2 = c_10; <--- this one
> >i_8 = i_11;
> >d_6 = i_11 + 1;
> >if (i_8 != 0)
> > goto ; [64.00%]
> >else
> > goto ; [36.00%]
> > ;;succ: 4
> > ;;6
> >
> > ;; basic block 4, loop depth 1
> > ;;pred: 3
> >if (d_6 <= c_2)
> > goto ; [0.00%]
> >else
> > goto ; [100.00%]
> >
> > we try to coalesce both c_10 to d_6 and i_11 to d_6, both have the same
> > cost I think and we succeed with the first which happens to be the one with
> > the default def arg.
> >
> > I also think whether we coalesce or not doesn't really matter for the issue
> > at
> > hand, the copy on entry should be elided anyway but the odd inserted copy
> > should be investigated (it looks unnecessary and it should be placed before
> > the single-use, not in the header).
> >
>
> Thanks for looking into this in detail, unfortunately I'm not familiar
> with this part of the compiler so I can't really comment on your findings.
>
> >> By disabling coalesce_ssa_name, we get the more usual copies on the
> >> incoming
> >> edges. The copy on the loop entry path still does an uninitialized read,
> >> but
> >> that one's now initialized by init-regs. The test-case passes, also when
> >> disabling init-regs, so it's possible that the JIT driver doesn't object to
> >> this type of uninitialized read.
> >>
> >> Now that we characterized the problem to some degree, we need to fix this,
> >> because either:
> >> - we're violating an undocumented ptx invariant, and this is a compiler
> >> bug,
> >>or
> >> - this is is a driver JIT bug and we need to work around it.
> >
> > So what does the JIT do that ends up breaking things? Does the
> > actual hardware ISA have NaTs and trap?
> >
>
> That's a good question. I haven't studied the actual hardware in
> detail, so I can't answer that. [ And the driver being closed source and
> the hardware undocumented by nvidia doesn't help in quickly finding a
> reliable answer there. ]
>
> However, my theory is the following: there's one or several
> optimizations in the JIT that takes a read from an unitialized register
> as sufficient proof to delete (some) depend insns.
>
> We've seen this in action before. The following is documented at
> WORKAROUND_PTXJIT_BUG in nvptx.cc.
>
> Consider this code:
> ...
> {
> .reg .u32 %x;
>
> mov.u32 %x,%tid.x;
>
> setp.ne.u32 %rnotvzero,%x,0;
>
> }
>
> @%rnotvzero bra Lskip;
> setp.. %rcond,op1,op2;
> Lskip:
>
Re: [RFC][nvptx] Initialize ptx regs
On 2/21/22 08:54, Richard Biener wrote:
On Sun, Feb 20, 2022 at 11:50 PM Tom de Vries via Gcc-patches
wrote:
Hi,
With nvptx target, driver version 510.47.03 and board GT 1030 I, we run into:
...
FAIL: gcc.c-torture/execute/pr53465.c -O1 execution test
FAIL: gcc.c-torture/execute/pr53465.c -O2 execution test
FAIL: gcc.c-torture/execute/pr53465.c -O3 -g execution test
...
while the test-cases pass with nvptx-none-run -O0.
The problem is that the generated ptx contains a read from an uninitialized
ptx register, and the driver JIT doesn't handle this well.
For -O2 and -O3, we can get rid of the FAIL using --param
logical-op-non-short-circuit=0. But not for -O1.
At -O1, the test-case minimizes to:
...
void __attribute__((noinline, noclone))
foo (int y) {
int c;
for (int i = 0; i < y; i++)
{
int d = i + 1;
if (i && d <= c)
__builtin_abort ();
c = d;
}
}
int main () {
foo (2); return 0;
}
...
Note that the test-case does not contain an uninitialized use. In the first
iteration, i is 0 and consequently c is not read. In the second iteration, c
is read, but by that time it's already initialized by 'c = d' from the first
iteration.
AFAICT the problem is introduced as follows: the conditional use of c in the
loop body is translated into an unconditional use of c in the loop header:
...
# c_1 = PHI
...
which forwprop1 propagates the 'c_9 = d_7' assignment into:
...
# c_1 = PHI
...
which ends up being translated by expand into an unconditional:
...
(insn 13 12 0 (set (reg/v:SI 22 [ c ])
(reg/v:SI 23 [ d ])) -1
(nil))
...
at the start of the loop body, creating an uninitialized read of d on the
path from loop entry.
Ah, interesting case. Note that some fixup pass inserted a copy in
the loop header
before coalescing:
;; basic block 3, loop depth 1
;;pred: 6
;;2
# c_10 = PHI
# i_11 = PHI
c_2 = c_10; <--- this one
i_8 = i_11;
d_6 = i_11 + 1;
if (i_8 != 0)
goto ; [64.00%]
else
goto ; [36.00%]
;;succ: 4
;;6
;; basic block 4, loop depth 1
;;pred: 3
if (d_6 <= c_2)
goto ; [0.00%]
else
goto ; [100.00%]
we try to coalesce both c_10 to d_6 and i_11 to d_6, both have the same
cost I think and we succeed with the first which happens to be the one with
the default def arg.
I also think whether we coalesce or not doesn't really matter for the issue at
hand, the copy on entry should be elided anyway but the odd inserted copy
should be investigated (it looks unnecessary and it should be placed before
the single-use, not in the header).
Thanks for looking into this in detail, unfortunately I'm not familiar
with this part of the compiler so I can't really comment on your findings.
By disabling coalesce_ssa_name, we get the more usual copies on the incoming
edges. The copy on the loop entry path still does an uninitialized read, but
that one's now initialized by init-regs. The test-case passes, also when
disabling init-regs, so it's possible that the JIT driver doesn't object to
this type of uninitialized read.
Now that we characterized the problem to some degree, we need to fix this,
because either:
- we're violating an undocumented ptx invariant, and this is a compiler bug,
or
- this is is a driver JIT bug and we need to work around it.
So what does the JIT do that ends up breaking things? Does the
actual hardware ISA have NaTs and trap?
That's a good question. I haven't studied the actual hardware in
detail, so I can't answer that. [ And the driver being closed source and
the hardware undocumented by nvidia doesn't help in quickly finding a
reliable answer there. ]
However, my theory is the following: there's one or several
optimizations in the JIT that takes a read from an unitialized register
as sufficient proof to delete (some) depend insns.
We've seen this in action before. The following is documented at
WORKAROUND_PTXJIT_BUG in nvptx.cc.
Consider this code:
...
{
.reg .u32 %x;
mov.u32 %x,%tid.x;
setp.ne.u32 %rnotvzero,%x,0;
}
@%rnotvzero bra Lskip;
setp.. %rcond,op1,op2;
Lskip:
selp.u32 %rcondu32,1,0,%rcond;
shfl.idx.b32 %rcondu32,%rcondu32,0,31;
setp.ne.u32 %rcond,%rcondu32,0;
...
My interpretation of what is supposed to happen, as per the ptx
specification is as follows.
The conditional setp. sets register %rcond, but just for thread 0 in
the warp (of 32 threads). The register remains uninitialized for
threads 1..31.
The selp sets register %rcondu32. For thread 0, it'll contain a defined
value, but for threads 1..31 it'll have undefined values.
The shfl propagates the value of thread 0 in register %rcondu32 to
threads 0..31. The register
Re: [RFC][nvptx] Initialize ptx regs
On Sun, Feb 20, 2022 at 11:50 PM Tom de Vries via Gcc-patches
wrote:
>
> Hi,
>
> With nvptx target, driver version 510.47.03 and board GT 1030 I, we run into:
> ...
> FAIL: gcc.c-torture/execute/pr53465.c -O1 execution test
> FAIL: gcc.c-torture/execute/pr53465.c -O2 execution test
> FAIL: gcc.c-torture/execute/pr53465.c -O3 -g execution test
> ...
> while the test-cases pass with nvptx-none-run -O0.
>
> The problem is that the generated ptx contains a read from an uninitialized
> ptx register, and the driver JIT doesn't handle this well.
>
> For -O2 and -O3, we can get rid of the FAIL using --param
> logical-op-non-short-circuit=0. But not for -O1.
>
> At -O1, the test-case minimizes to:
> ...
> void __attribute__((noinline, noclone))
> foo (int y) {
> int c;
> for (int i = 0; i < y; i++)
> {
> int d = i + 1;
> if (i && d <= c)
> __builtin_abort ();
> c = d;
> }
> }
>
> int main () {
> foo (2); return 0;
> }
> ...
>
> Note that the test-case does not contain an uninitialized use. In the first
> iteration, i is 0 and consequently c is not read. In the second iteration, c
> is read, but by that time it's already initialized by 'c = d' from the first
> iteration.
>
> AFAICT the problem is introduced as follows: the conditional use of c in the
> loop body is translated into an unconditional use of c in the loop header:
> ...
> # c_1 = PHI
> ...
> which forwprop1 propagates the 'c_9 = d_7' assignment into:
> ...
> # c_1 = PHI
> ...
> which ends up being translated by expand into an unconditional:
> ...
> (insn 13 12 0 (set (reg/v:SI 22 [ c ])
> (reg/v:SI 23 [ d ])) -1
> (nil))
> ...
> at the start of the loop body, creating an uninitialized read of d on the
> path from loop entry.
Ah, interesting case. Note that some fixup pass inserted a copy in
the loop header
before coalescing:
;; basic block 3, loop depth 1
;;pred: 6
;;2
# c_10 = PHI
# i_11 = PHI
c_2 = c_10; <--- this one
i_8 = i_11;
d_6 = i_11 + 1;
if (i_8 != 0)
goto ; [64.00%]
else
goto ; [36.00%]
;;succ: 4
;;6
;; basic block 4, loop depth 1
;;pred: 3
if (d_6 <= c_2)
goto ; [0.00%]
else
goto ; [100.00%]
we try to coalesce both c_10 to d_6 and i_11 to d_6, both have the same
cost I think and we succeed with the first which happens to be the one with
the default def arg.
I also think whether we coalesce or not doesn't really matter for the issue at
hand, the copy on entry should be elided anyway but the odd inserted copy
should be investigated (it looks unnecessary and it should be placed before
the single-use, not in the header).
> By disabling coalesce_ssa_name, we get the more usual copies on the incoming
> edges. The copy on the loop entry path still does an uninitialized read, but
> that one's now initialized by init-regs. The test-case passes, also when
> disabling init-regs, so it's possible that the JIT driver doesn't object to
> this type of uninitialized read.
>
> Now that we characterized the problem to some degree, we need to fix this,
> because either:
> - we're violating an undocumented ptx invariant, and this is a compiler bug,
> or
> - this is is a driver JIT bug and we need to work around it.
So what does the JIT do that ends up breaking things? Does the
actual hardware ISA have NaTs and trap?
> There are essentially two strategies to address this:
> - stop the compiler from creating uninitialized reads
> - patch up uninitialized reads using additional initialization
>
> The former will probably involve:
> - making some optimizations more conservative in the presence of
> uninitialized reads, and
> - disabling some other optimizations (where making them more conservative is
> not possible, or cannot easily be achieved).
> This will probably will have a cost penalty for code that does not suffer from
> the original problem.
>
> The latter has the problem that it may paper over uninitialized reads
> in the source code, or indeed over ones that were incorrectly introduced
> by the compiler. But it has the advantage that it allows for the problem to
> be addressed at a single location.
There are some long-standing bug in bugzilla regarding to uninit uses
and how we treat them as invoking undefined behavior but also introduce
those ourselves in some places. We of course can't do both so I think
we do neet to get our hands at a way to fix things without introducing
too many optimization regressions.
You've identified the most obvious candidate already - logical-op
short-circuiting.
I guess that the PTX JIT is fine with uninitialized memory so one issue
is that we can end up turning uninitialized memory into uninitialized
registers (not changing the point of execution though), if the JIT will
break here you will need to fixup in reorg like you do.
> There's an existing pass, init-regs, which implements a form of the latter,
>
[RFC][nvptx] Initialize ptx regs
Hi,
With nvptx target, driver version 510.47.03 and board GT 1030 I, we run into:
...
FAIL: gcc.c-torture/execute/pr53465.c -O1 execution test
FAIL: gcc.c-torture/execute/pr53465.c -O2 execution test
FAIL: gcc.c-torture/execute/pr53465.c -O3 -g execution test
...
while the test-cases pass with nvptx-none-run -O0.
The problem is that the generated ptx contains a read from an uninitialized
ptx register, and the driver JIT doesn't handle this well.
For -O2 and -O3, we can get rid of the FAIL using --param
logical-op-non-short-circuit=0. But not for -O1.
At -O1, the test-case minimizes to:
...
void __attribute__((noinline, noclone))
foo (int y) {
int c;
for (int i = 0; i < y; i++)
{
int d = i + 1;
if (i && d <= c)
__builtin_abort ();
c = d;
}
}
int main () {
foo (2); return 0;
}
...
Note that the test-case does not contain an uninitialized use. In the first
iteration, i is 0 and consequently c is not read. In the second iteration, c
is read, but by that time it's already initialized by 'c = d' from the first
iteration.
AFAICT the problem is introduced as follows: the conditional use of c in the
loop body is translated into an unconditional use of c in the loop header:
...
# c_1 = PHI
...
which forwprop1 propagates the 'c_9 = d_7' assignment into:
...
# c_1 = PHI
...
which ends up being translated by expand into an unconditional:
...
(insn 13 12 0 (set (reg/v:SI 22 [ c ])
(reg/v:SI 23 [ d ])) -1
(nil))
...
at the start of the loop body, creating an uninitialized read of d on the
path from loop entry.
By disabling coalesce_ssa_name, we get the more usual copies on the incoming
edges. The copy on the loop entry path still does an uninitialized read, but
that one's now initialized by init-regs. The test-case passes, also when
disabling init-regs, so it's possible that the JIT driver doesn't object to
this type of uninitialized read.
Now that we characterized the problem to some degree, we need to fix this,
because either:
- we're violating an undocumented ptx invariant, and this is a compiler bug,
or
- this is is a driver JIT bug and we need to work around it.
There are essentially two strategies to address this:
- stop the compiler from creating uninitialized reads
- patch up uninitialized reads using additional initialization
The former will probably involve:
- making some optimizations more conservative in the presence of
uninitialized reads, and
- disabling some other optimizations (where making them more conservative is
not possible, or cannot easily be achieved).
This will probably will have a cost penalty for code that does not suffer from
the original problem.
The latter has the problem that it may paper over uninitialized reads
in the source code, or indeed over ones that were incorrectly introduced
by the compiler. But it has the advantage that it allows for the problem to
be addressed at a single location.
There's an existing pass, init-regs, which implements a form of the latter,
but it doesn't work for this example because it only inserts additional
initialization for uses that have not a single reaching definition.
Fix this by adding initialization of uninitialized ptx regs in reorg.
Control the new functionality using -minit-regs=<0|1|2|3>, meaning:
- 0: disabled.
- 1: add initialization of all regs at the entry bb
- 2: add initialization of uninitialized regs at the entry bb
- 3: add initialization of uninitialized regs close to the use
and defaulting to 3.
Tested on nvptx.
Any comments?
Thanks,
- Tom
[nvptx] Initialize ptx regs
gcc/ChangeLog:
2022-02-17 Tom de Vries
PR target/104440
* config/nvptx/nvptx.cc (workaround_uninit_method_1)
(workaround_uninit_method_2, workaround_uninit_method_3)
(workaround_uninit): New function.
(nvptx_reorg): Use workaround_uninit.
* config/nvptx/nvptx.opt (minit-regs): New option.
---
gcc/config/nvptx/nvptx.cc | 188 +
gcc/config/nvptx/nvptx.opt | 4 +
2 files changed, 192 insertions(+)
diff --git a/gcc/config/nvptx/nvptx.cc b/gcc/config/nvptx/nvptx.cc
index ed347cab70e..a37a6c78b41 100644
--- a/gcc/config/nvptx/nvptx.cc
+++ b/gcc/config/nvptx/nvptx.cc
@@ -5372,6 +5372,190 @@ workaround_barsyncs (void)
}
#endif
+/* Initialize all declared regs at function entry.
+ Advantage : Fool-proof.
+ Disadvantage: Potentially creates a lot of long live ranges and adds a lot
+of insns. */
+
+static void
+workaround_uninit_method_1 (void)
+{
+ rtx_insn *first = get_insns ();
+ rtx_insn *insert_here = NULL;
+
+ for (int ix = LAST_VIRTUAL_REGISTER + 1; ix < max_reg_num (); ix++)
+{
+ rtx reg = regno_reg_rtx[ix];
+
+ /* Skip undeclared registers. */
+ if (reg == const0_rtx)
+ continue;
+
+ gcc_assert (CONST0_RTX (GET_MODE (reg)));
+
+ start_sequence ();
+ emit_move_insn (reg, CONST0_RTX (GET_MODE (reg)));
+ rtx_insn
