<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Reid,</p>
<blockquote type="cite"
cite="mid:CACs=tyJrNzz0NYJejYSOnAx=heRHT+936m9y9GBZJSATw2MmOw@mail.gmail.com">
<div dir="ltr">
<div>I don't think it would be reasonable for the SystemZ
backend to abort every time it is asked to code generate an
i32 argument that lacks an attribute. There are simply too
many IR producers (frontends and instrumentation tools) that
will pass unannotated i32 values to calls. It's not feasible
to fix them all, and aborting just means these producers will
have to be updated whenever they are ported to SystemZ or any
other target adopting the same practice.</div>
<div><br>
</div>
<div>I think it would instead be reasonable to pick a safer
default for the target, like always zero extending unannotated
parameters on both the caller side and the callee side. This
is less efficient than only extending once in the caller, but
it makes it much easier to retarget an IR producer over to
SystemZ. It makes the attributes into performance
improvements, not correctness requirements. I suspect this
will not actually break passing a 4 byte struct, it just makes
it less efficient.</div>
<div><br>
</div>
</div>
</blockquote>
<p>As Uli explained, this is not an optimization but actually really
necessary in order to perform the right extension. I don't think
there is any way around fixing this the slow and arduous way :-/</p>
<blockquote type="cite"
cite="mid:CACs=tyJrNzz0NYJejYSOnAx=heRHT+936m9y9GBZJSATw2MmOw@mail.gmail.com">
<div dir="ltr">
<div>As for the suggestion to add an explicit structarg
attribute, can this be generalized to an attribute that marks
the high bits as undef? So, IR producers will have a portable
way to explicitly request the current behavior. This is kind
of like `anyext`, but not really, more like `undefext`,
`uext`, `hiundef` or `calleemustextend`.</div>
<div><br>
</div>
<div>What do you think?</div>
</div>
</blockquote>
<p>I agree that some more general attribute could work as well to
mean for a parameter that it is not an integer value and no
extension is therefore required per the ABI. The important thing
about the name is that whoever builds the call instruction needs
to be aware that it is only for non-integer values - the other
trap to fall into I guess might be that 'anyext' could be added
carelessly on integers when it seems that it might not matter,
even when it really does per the ABI... :-) Maybe the name
'anyext' would make people think that it will be extended one way
or the other, which is not true. I think I like your 'hiundef'
better... Or maybe 'nonint', or 'noext'?<br>
</p>
<p>Do you agree we need to do this, and what would the next step be
towards getting this in place? If we decide on the attribute to
use, I could implement the assertion and start filing bugs and
hopefully get some help with them as well... Even though this may
not be "feasible", I hope this would eventually lead to having
this working correctly, or is there any reason not?</p>
<p>/Jonas<br>
</p>
<br>
<blockquote type="cite"
cite="mid:CACs=tyJrNzz0NYJejYSOnAx=heRHT+936m9y9GBZJSATw2MmOw@mail.gmail.com">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Sep 29, 2021 at 4:42
AM Jonas Paulsson <<a
href="mailto:paulsson@linux.vnet.ibm.com" target="_blank"
moz-do-not-send="true" class="moz-txt-link-freetext">paulsson@linux.vnet.ibm.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
We have many times now had severe problems relating to
parameter passing <br>
where the signext/zeroext attribute has been forgotten when
building the <br>
LLVM I/R call instruction somewhere during compilation. The
last example <br>
of this is MLIR: some function related to async runtime takes
an int32_t <br>
as argument, but the place that builds the call is not adding
the <br>
signext attribute on that parameter. As a result wrong code(!)
resulted <br>
on SystemZ and just out of luck we happened to discover this
when <br>
building MLIR with gcc (bug hides with clang). Details at <br>
<a href="https://bugs.llvm.org/show_bug.cgi?id=51898"
rel="noreferrer" target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://bugs.llvm.org/show_bug.cgi?id=51898</a>.<br>
<br>
The first question one might ask about this is why is there no
assert <br>
catching this in the SystemZ (or any other target with the
same ABI <br>
handling of arguments)? The reason is that there is a third
type of <br>
integer parameter: struct passed in register. So if there is
neither a <br>
signext or a zeroext attribute on an i32 argument, it must be
assumed <br>
that it contains a struct (of four bytes, for example).<br>
<br>
I personally feel there is something missing here and a shame
that this <br>
very serious problem lives on. I totally understand that
people working <br>
on a target that does not have this ABI requirement may not be
aware <br>
that their new code causes wrong-code on some platforms.
Therefore I <br>
think it all the more important that they see some test
failing as soon <br>
as they make the mistake.<br>
<br>
In order to implement the assert in the backend one idea might
be to <br>
give the struct-in-reg parameter an explicit attribute. Would
it be <br>
reasonable/doable to invent a new attribute such as StructArg
and add it <br>
on all such arguments throughout the whole LLVM code base?
Then we could <br>
assert for signext/zeroext/structarg and eliminate this type
of errors.<br>
<br>
Some examples:<br>
<br>
The SystemZ ABI requires that all integer parameters always
are to be <br>
extended to full register width (64-bit), either signed or
unsigned <br>
depending on the type.<br>
<br>
-- For exampe, this function is loading 32-bits which are
passed to <br>
another function:<br>
<br>
int bar(int *a) {<br>
B return foo(*a);<br>
}<br>
<br>
=> LLVM I/R:<br>
<br>
B %0 = load i32, i32* %a<br>
B %call = tail call signext i32 @foo(i32 signext %0)<br>
<br>
=> SystemZ MachineInstructions:<br>
<br>
B %1:gr64bit = LGF %0:addr64bit, 0, $noreg :: (load (s32)
from %ir.a, <br>
!tbaa !2)<br>
B $r2d = COPY %1:gr64bit<br>
B CallJG @foo, implicit $r2d<br>
<br>
The important mechanism here is that on LLVM I/R the 'signext'
attribute <br>
is added to the parameter %0. That way the backend knows that
a sign <br>
extension is needed (there is no other way to know the
signedness), and <br>
emits the sign-extending load (LGF).<br>
<br>
-- A matching example, on the callee side would be:<br>
<br>
long bar(int a) {<br>
B return ((long) a);<br>
}<br>
<br>
=><br>
<br>
B %conv = sext i32 %a to i64<br>
B ret i64 %conv<br>
<br>
=><br>
<br>
B Return implicit $r2d<br>
<br>
The 32-bit ingoing parameter is per the ABI required to
already have <br>
been sign-extended so the backend can simply do nothing. NB!
This leads <br>
to wrong code if the caller forgets about that :-)<br>
<br>
-- A struct in register:<br>
<br>
struct S {<br>
B short el0;<br>
B short el1;<br>
};<br>
<br>
int foo(struct S arg);<br>
<br>
void bar(short c) {<br>
B struct S a = {c, c + 1};<br>
B foo (a);<br>
}<br>
<br>
=> LLVM I/R<br>
<br>
define void @bar(i16 signext %c) local_unnamed_addr #0 {<br>
<br>
B %add = add i16 %c, 1<br>
B %a.sroa.4.0.insert.ext = zext i16 %add to i32<br>
B %a.sroa.0.0.insert.ext = zext i16 %c to i32<br>
B %a.sroa.0.0.insert.shift = shl nuw i32
%a.sroa.0.0.insert.ext, 16<br>
B %a.sroa.0.0.insert.insert = or i32
%a.sroa.0.0.insert.shift, <br>
%a.sroa.4.0.insert.ext<br>
B %call = tail call signext i32 @foo(i32
%a.sroa.0.0.insert.insert) #2<br>
B ret void<br>
}<br>
<br>
The i32 passed to foo does not have any attribute set, and the
backend <br>
does not extend it. An 'int' gets the same treatment without
the signext <br>
attribute set, which is then wrong... :-(<br>
<br>
/Jonas<br>
<br>
<br>
</blockquote>
</div>
</blockquote>
</body>
</html>