<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 01/20/2013 10:56 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0Kju_szMrLyoGo5JGh6MW-TWAhVzTu2AAZALJm7ZYUB3zg@mail.gmail.com"
type="cite">
<div dir="ltr">I doubt you needed to add cfe-dev here. Sorry I
hadn't seen this, this seems like an easy and simple deficiency
in the IR intrinsic for memcpy. See below.
<div class="gmail_extra"><br>
<div class="gmail_quote">
On Sun, Jan 20, 2013 at 1:42 PM, Arnaud de Grandmaison <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:arnaud.allarddegrandmaison@parrot.com"
target="_blank" class="cremed">arnaud.allarddegrandmaison@parrot.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">define
void @test(i16 zeroext %a) nounwind uwtable {<br>
%r.sroa.0 = alloca i16, align 2<br>
%r.sroa.1 = alloca i16, align 2<br>
store i16 %a, i16* %r.sroa.0, align 2<br>
store i16 1, i16* %r.sroa.1, align 2<br>
%r.sroa.0.0.load3 = load volatile i16* %r.sroa.0, align
2<br>
store volatile i16 %r.sroa.0.0.load3, i16* inttoptr (i64
416 to i16*), align 32<br>
%r.sroa.1.0.load2 = load volatile i16* %r.sroa.1, align
2<br>
store volatile i16 %r.sroa.1.0.load2, i16* inttoptr (i64
418 to i16*), align 2<br>
</blockquote>
<div><br>
</div>
<div>
<div>Just so we're on the same page, the "regression"
(which I claim is actually a bug-fix) are the loads here
being volatile in addition to the stores. The stores
require it, but the loads get it.</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
We are on the same page there. And although this shows as a
performance regression with my targets, I agree this is a bug fix,
especially given the actual semantics of the memcpy intrinsic.<br>
<br>
<blockquote
cite="mid:CAGCO0Kju_szMrLyoGo5JGh6MW-TWAhVzTu2AAZALJm7ZYUB3zg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>
</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">The
problem is in how clang codegen aggregate copies when the
source or<br>
the destination are volatile : it uses the memcpy
intrinsic, setting the<br>
volatile parameter. However, the LLVM IR reference manual
specifies that<br>
"The detailed access behavior is not very cleanly
specified and it is<br>
unwise to depend on it". The new SROA implementation is
conservatively<br>
correct, but behaves differently.<br>
<br>
I believe we should either :<br>
- rework methods AggExprEmitter::EmitCopy and<br>
CodeGenFunction::EmitAggregateCopy (from<br>
clang/lib/CodeGen/CGExprAgg.cpp) to codegen differently
the copy when<br>
either the source or the destination is volatile,<br>
</blockquote>
<div><br>
</div>
<div style="">I think this is bad...</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> -
specify cleanly the memcpy intrinsic with respect to
volatile behaviour<br>
</blockquote>
<div><br>
</div>
<div style="">I agree that this is what we should do.</div>
<div style=""><br>
</div>
<div style="">I took a conservative approach when
implementing SROA w.r.t. volatile memcpy, and I think we
should codify those semantics: with a single is-volatile
bit both the loads and stores which the memcpy decomposes
into will have volatile semantics.</div>
<div style=""><br>
</div>
<div style="">I think we should extend the memcpy intrinsic
to support two volatile flags and have that overload have
the flags refer to the loads and stores respectively. We
should then auto-upgrade the existing memcpy intrinsics
into the more precise form by replicating the single
volatile flag. This will conservatively preserve
semantics. As part of adding the new intrinsic, everywhere
in LLVM that examines the two flags should be audited and
should use the individual flag for load vs. store
volatility. This will change SROA so that it can emit the
loads as non-volatile, and only the stores will be
volatile. Finally we switch Clang over to emit the new
intrinsic structure with more precise information, and
your test case should go back to normal.</div>
<div style=""><br>
</div>
<div style="">As part of this, we should also clarify that a
volatile memcpy guarantees that each byte loaded is loaded
exactly once, and each byte stored is stored exactly once,
but that these operations are free to be batched into
4-byte, 8-byte, or any other width of loads and stores,
and there is no guarantee about the interleaving of these
loads and stores even when both are volatile.</div>
<div style=""><br>
</div>
<div style="">-Chandler</div>
</div>
</div>
</div>
</blockquote>
<br>
I am volunteering to implement this. I have to finish my code
porting to llvm-3.2 first ; expect some patches / discussions by the
end of the week.<br>
<br>
Cheers,<br>
<pre class="moz-signature" cols="72">--
Arnaud de Grandmaison
</pre>
</body>
</html>