[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In https://reviews.llvm.org/D30415#705889, @echristo wrote:

> In https://reviews.llvm.org/D30415#705196, @uweigand wrote:
>
> > Well, mainline GCC doesn't have -faltivec at all and never had, I think 
> > this was only an Apple GCC extension ...  Not sure what exactly the 
> > semantics of that was.
>
>
> Sure it does and has for years. Check out rs6000/darwin.h :)
>
> FWIW: It turns on maltivec and adds a -include of altivec.h


Huh, I wasn't aware of that feature on Darwin, thanks for pointing it out ...

> Nearly all of the code in lib/Driver/ToolChains/Clang.cpp and 
> lib/Driver/ToolChains/Arch/PPC.cpp that deal with altivec. Simplifying the 
> interface by getting rid of needing to check multiple options.

But why would that code no longer be necessary for -maltivec?  Well, I guess 
I'll wait for your patch ...

If we indeed get rid of -faltivec, I'm wondering whether it would also make 
sense to get rid of -fzvector.  This is just an alias for -mzvector, and it 
isn't supported by GCC either.  I added it only because Richard Smith 
specifically asked for it when I contributed the feature here:
https://reviews.llvm.org/D11001

> This should be a -f flag, not a -m flag. (I think we only support -maltivec 
> for GCC compatibility.)




https://reviews.llvm.org/D30415



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In https://reviews.llvm.org/D30415#704761, @echristo wrote:

> In https://reviews.llvm.org/D30415#703652, @uweigand wrote:
>
> > I'm a bit confused by this discussion.  -faltivec and -maltivec are simply 
> > aliases, they do exactly the same thing; the clang-internal variable 
> > OPT_faltivec indicates the use of either -faltivec or -maltivec.
>
>
> They didn't used to, I arranged it so that they did (technically breaking gcc 
> compatibility) a while ago.


Well, mainline GCC doesn't have -faltivec at all and never had, I think this 
was only an Apple GCC extension ...  Not sure what exactly the semantics of 
that was.

>> Or is the suggestion to simply remove the alias -faltivec, and leave 
>> -maltivec as-is?  I'd be less opposed to this since it probably breaks fewer 
>> users ... but I'm still not quite sure what it actually buys us.   And in 
>> any case the patch currently under discussion here would still be necessary 
>> then, to fix -maltivec -mno-altivec ...
> 
> No, remove faltivec and move forward with -maltivec/-mno-altivec but you 
> should be able to remove a lot of the special handling at that point.

I'm still confused as to what exactly you're refering to here.  As far as I can 
see, every single thing triggered by -faltivec / -maltivec in the compiler 
frontend would still be needed exactly the same if we only supported the 
-maltivec option name.  So the only thing we'd save is literally the two lines 
in include/clang/Driver/Options.td that set up the alias.

Do you have an example of the "special handling" to remove you're thinking of?


https://reviews.llvm.org/D30415



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In https://reviews.llvm.org/D30415#703442, @hfinkel wrote:

> In https://reviews.llvm.org/D30415#703398, @echristo wrote:
>
> > Different suggestion:
> >
> > Remove the faltivec option. Even gcc doesn't support it anymore afaict.
>
>
> What are you suggesting? Always having the language extensions on? Or 
> explicitly tying the language extensions to the underlying target feature?


I'm a bit confused by this discussion.  -faltivec and -maltivec are simply 
aliases, they do exactly the same thing; the clang-internal variable 
OPT_faltivec indicates the use of either -faltivec or -maltivec.

Is the suggestion to remove that flag completely, i.e. both -maltivec and 
-faltivec?   This seems strange to me since -maltivec is used in many Makefiles 
etc. that would break if clang suddenly refused to accept the option.

Or is the suggestion to simply remove the alias -faltivec, and leave -maltivec 
as-is?  I'd be less opposed to this since it probably breaks fewer users ... 
but I'm still not quite sure what it actually buys us.   And in any case the 
patch currently under discussion here would still be necessary then, to fix 
-maltivec -mno-altivec ...


https://reviews.llvm.org/D30415



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33353: [OpenCL] An error shall occur if any scalar operand has greater rank than the type of the vector element

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Looks like this causes the build bot to fail on SystemZ:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/8741

error: 'error' diagnostics expected but not seen:

  File 
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/SemaOpenCL/arithmetic-conversions.cl
 Line 7: scalar operand type has greater rank than the type of the vector 
element. ('float2' (vector of 2 'float' values) and 'double')
  File 
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/SemaOpenCL/arithmetic-conversions.cl
 Line 9: scalar operand type has greater rank than the type of the vector 
element. ('double' and 'float2' (vector of 2 'float' values))

error: 'warning' diagnostics seen but not expected:

  File 
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/SemaOpenCL/arithmetic-conversions.cl
 Line 7: double precision constant requires cl_khr_fp64, casting to single 
precision
  File 
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/SemaOpenCL/arithmetic-conversions.cl
 Line 9: double precision constant requires cl_khr_fp64, casting to single 
precision


https://reviews.llvm.org/D33353



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33353: [OpenCL] An error shall occur if any scalar operand has greater rank than the type of the vector element

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In https://reviews.llvm.org/D33353#765783, @bader wrote:

> Could you revert Egor's commit, please?


I see Renato Golin has already reverted the commit, thanks ...


https://reviews.llvm.org/D33353



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33353: [OpenCL] An error shall occur if any scalar operand has greater rank than the type of the vector element

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

The problem is that the new test case you added does not contain a -triple 
argument in the compile command.  This means that the compile targets the 
native architecture on the build system, whatever this is.  Since OpenCL 
support on different architectures may be different (e.g. some may support the 
cl_khr_fp64 extension by default while others do not), you see the error only 
on some build architectures.

If you want to see the error, I guess you can force targeting SystemZ by adding 
"-triple s390x-ibm-linux-gnu" to the compile command.

To actually fix the problem, as mentioned above, add a -triple line that 
specifies an architecture where the extension is always known to be present.  
(Or else, I think you can force the extension to be available even if it is not 
by default on a target.)


https://reviews.llvm.org/D33353



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33648: [OpenCL] An error shall occur if any scalar operand has greater rank than the type of the vector element

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

Yes, this works for me now.  Thanks!


https://reviews.llvm.org/D33648



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52840: Include Python binding tests in CMake rules

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In https://reviews.llvm.org/D52840#1265615, @mgorny wrote:

> The first one seems to indicate that your `libclang.so` is broken in release 
> mode (optimization error?). The second one is correct (some of the tests test 
> for errors, and apparently don't silence the messages).


Ok, thanks for the clarification on the second point.

As to the first issue, it turned out to be more complicated.  The wrong value 
results from this call in CursorVisitor::Visit

  switch (Visitor(Cursor, Parent, ClientData)) {

This is a callback routine passed in from the caller, which in the case of the 
Python bindings means that an FFI closure is being called.

Now, due to a historical quirk in the FFI ABI, return values of integral type 
smaller than the register word size must be passed as the special "ffi_arg" 
type when using FFI (either for calls or for closures).  It seems the Python 
2.7 code that interfaces with FFI does not correctly respect this.  As a 
result, it seems that when the closure returns a 32-bit value, it is not 
properly extended and the high bits of the return register contain garbage.

But that violates our ABI, which requires extension to full register size.   
And it seems the clang code, when built with optimization, does indeed rely on 
that ABI guarantee.

So there's a bug in Python (or depending on how you want to look at it, in 
libffi -- that part of the interface has also long been under-documented 
unfortunately), which hits on our platform.  I'll see if I can do anything 
about that, but for now we'll probably want to disable those tests on SystemZ.


Repository:
  rC Clang

https://reviews.llvm.org/D52840



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52840: Include Python binding tests in CMake rules

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

This causes check-all to abort for me on SystemZ in Release mode (and never 
actually run the lit tests):

  [40/365] cd /home/uweigand/llvm/llvm-head/tools/clang/bindings/python && 
/usr/bin/cmake -E...BRARY_PATH=/home/uweigand/llvm/build/llvm-head/lib 
/usr/bin/python2.7 -m unittest discover
  FAILED: tools/clang/bindings/python/tests/CMakeFiles/check-clang-python 
  cd /home/uweigand/llvm/llvm-head/tools/clang/bindings/python && 
/usr/bin/cmake -E env 
CLANG_LIBRARY_PATH=/home/uweigand/llvm/build/llvm-head/lib /usr/bin/python2.7 
-m unittest discover
  Invalid CXChildVisitResult!
  UNREACHABLE executed at 
/home/uweigand/llvm/llvm-head/tools/clang/tools/libclang/CIndex.cpp:233!
  Child aborted

When running in Debug mode I see instead:

  [165/536] cd /home/uweigand/llvm/llvm-head/tools/clang/bindings/python && 
/usr/bin/cmake -...PATH=/home/uweigand/llvm/build/llvm-head-debug/lib 
/usr/bin/python2.7 -m unittest discover
  ..LIBCLANG 
TOOLING ERROR: fixed-compilation-database: Error while opening fixed database: 
No such file or directory
  json-compilation-database: Error while opening JSON database: No such file or 
directory
  
  ..
  --
  Ran 120 tests in 8.522s
  
  OK

which apparently doesn't count as failure, but still doesn't look quite right 
to me.

Is this some environment setup problem, or does this expose a real bug?  It's 
certainly not good that running the remaining tests is completely aborted in 
Release mode ...


Repository:
  rC Clang

https://reviews.llvm.org/D52840



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55916: [clang] Replace getOS() == llvm::Triple::*BSD with isOS*BSD() [NFCI]

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D55916#1337427 , @uweigand wrote:

> This causes test case failures due to no longer linking with -lrt on Linux.


Never mind, already fixed :-)   Thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55916/new/

https://reviews.llvm.org/D55916



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55916: [clang] Replace getOS() == llvm::Triple::*BSD with isOS*BSD() [NFCI]

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

This causes test case failures due to no longer linking with -lrt on Linux.




Comment at: lib/Driver/ToolChains/CommonArgs.cpp:606
 CmdArgs.push_back("-lpthread");
-if (TC.getTriple().getOS() != llvm::Triple::OpenBSD)
+if (TC.getTriple().isOSOpenBSD())
   CmdArgs.push_back("-lrt");

Looks like this is missing a ! here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55916/new/

https://reviews.llvm.org/D55916



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

jyknight wrote:
> Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
> InitPreprocessor alongside the rest of the SIZEOF macros?
> 
> And then use that to determine whether to add float128 to the union? This 
> change, as is, will break on any target which is i386 but doesn't define 
> __float128, e.g. i386-unknown-unknown.
> 
> The only additional target which will be modified with that (that is: the 
> only other target which has a float128 type, but for which max_align isn't 
> already 16) is systemz-*-linux.
> 
> But I think that's actually indicating a pre-existing bug in the SystemZ 
> config -- it's configured for a 16-byte long double, with 8-byte alignment, 
> but the ABI doc seems to call for 16-byte alignment. +Ulrich for comment on 
> that.
That's a bug in the ABI doc which we'll fix once we get around to releasing an 
updated version.

long double on SystemZ must be 8-byte aligned, which is the maximum alignment 
of all standard types on Z, and this was chosen because historically the ABI 
only guarantees an 8-byte stack alignment, so 16-byte aligned standard types 
would be awkward.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55057/new/

https://reviews.llvm.org/D55057



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

jyknight wrote:
> EricWF wrote:
> > uweigand wrote:
> > > jyknight wrote:
> > > > uweigand wrote:
> > > > > jyknight wrote:
> > > > > > Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
> > > > > > InitPreprocessor alongside the rest of the SIZEOF macros?
> > > > > > 
> > > > > > And then use that to determine whether to add float128 to the 
> > > > > > union? This change, as is, will break on any target which is i386 
> > > > > > but doesn't define __float128, e.g. i386-unknown-unknown.
> > > > > > 
> > > > > > The only additional target which will be modified with that (that 
> > > > > > is: the only other target which has a float128 type, but for which 
> > > > > > max_align isn't already 16) is systemz-*-linux.
> > > > > > 
> > > > > > But I think that's actually indicating a pre-existing bug in the 
> > > > > > SystemZ config -- it's configured for a 16-byte long double, with 
> > > > > > 8-byte alignment, but the ABI doc seems to call for 16-byte 
> > > > > > alignment. +Ulrich for comment on that.
> > > > > That's a bug in the ABI doc which we'll fix once we get around to 
> > > > > releasing an updated version.
> > > > > 
> > > > > long double on SystemZ must be 8-byte aligned, which is the maximum 
> > > > > alignment of all standard types on Z, and this was chosen because 
> > > > > historically the ABI only guarantees an 8-byte stack alignment, so 
> > > > > 16-byte aligned standard types would be awkward.
> > > > Then perhaps it's a bug that `__alignof__(__float128)` returns 16 with 
> > > > `-target systemz-linux`?
> > > Huh, really __float128 should not be supported at all on SystemZ.  It's 
> > > not supported with GCC either (GCC never provides __float128 on targets 
> > > where long double is already IEEE-128).  (GCC does support the new 
> > > _Float128 on SystemZ, but that's not yet supported by clang anywhere.)
> > > 
> > > If it were supported, I agree it should be an alias for long double, and 
> > > also have an alignof of 8.
> > @jyknight Ack. I'll add `__SIZEOF_FLOAT128__`.
> @uweigand : One of those fixes needs to land before this, so that systemz's 
> max_align_t doesn't change to 16 in the meantime. I think your preference 
> would be for it to be simply removed, right? Looks like the type was 
> originally added in https://reviews.llvm.org/D19125 -- possibly in error, 
> since the focus was x86_64.
@jyknight : Yes, this seems to have been simply an error.  I'll check in a 
patch to remove `__float128` on SystemZ.



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55057/new/

https://reviews.llvm.org/D55057



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand marked an inline comment as done.
uweigand added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

uweigand wrote:
> jyknight wrote:
> > EricWF wrote:
> > > uweigand wrote:
> > > > jyknight wrote:
> > > > > uweigand wrote:
> > > > > > jyknight wrote:
> > > > > > > Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
> > > > > > > InitPreprocessor alongside the rest of the SIZEOF macros?
> > > > > > > 
> > > > > > > And then use that to determine whether to add float128 to the 
> > > > > > > union? This change, as is, will break on any target which is i386 
> > > > > > > but doesn't define __float128, e.g. i386-unknown-unknown.
> > > > > > > 
> > > > > > > The only additional target which will be modified with that (that 
> > > > > > > is: the only other target which has a float128 type, but for 
> > > > > > > which max_align isn't already 16) is systemz-*-linux.
> > > > > > > 
> > > > > > > But I think that's actually indicating a pre-existing bug in the 
> > > > > > > SystemZ config -- it's configured for a 16-byte long double, with 
> > > > > > > 8-byte alignment, but the ABI doc seems to call for 16-byte 
> > > > > > > alignment. +Ulrich for comment on that.
> > > > > > That's a bug in the ABI doc which we'll fix once we get around to 
> > > > > > releasing an updated version.
> > > > > > 
> > > > > > long double on SystemZ must be 8-byte aligned, which is the maximum 
> > > > > > alignment of all standard types on Z, and this was chosen because 
> > > > > > historically the ABI only guarantees an 8-byte stack alignment, so 
> > > > > > 16-byte aligned standard types would be awkward.
> > > > > Then perhaps it's a bug that `__alignof__(__float128)` returns 16 
> > > > > with `-target systemz-linux`?
> > > > Huh, really __float128 should not be supported at all on SystemZ.  It's 
> > > > not supported with GCC either (GCC never provides __float128 on targets 
> > > > where long double is already IEEE-128).  (GCC does support the new 
> > > > _Float128 on SystemZ, but that's not yet supported by clang anywhere.)
> > > > 
> > > > If it were supported, I agree it should be an alias for long double, 
> > > > and also have an alignof of 8.
> > > @jyknight Ack. I'll add `__SIZEOF_FLOAT128__`.
> > @uweigand : One of those fixes needs to land before this, so that systemz's 
> > max_align_t doesn't change to 16 in the meantime. I think your preference 
> > would be for it to be simply removed, right? Looks like the type was 
> > originally added in https://reviews.llvm.org/D19125 -- possibly in error, 
> > since the focus was x86_64.
> @jyknight : Yes, this seems to have been simply an error.  I'll check in a 
> patch to remove `__float128` on SystemZ.
> 
Checked in as rev. 348247.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55057/new/

https://reviews.llvm.org/D55057



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

As an aside, it would be nice if we had a test case that verifies the explicit 
values of alignof(max_align_t) on all supported platforms.  This is an ABI 
property that should never change.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55057/new/

https://reviews.llvm.org/D55057



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D53157#1309743 , @cameron.mcinally 
wrote:

> Digressing a bit, but has anyone given thought to how this implementation 
> will play out with libraries? When running with traps enabled, libraries must 
> be compiled for trap-safety. E.g. optimizations on a lib's code could 
> introduce a NaN or cause a trap that does not exist in the code itself.


In my reading of the standard text, there is no special provision for library 
code.  This means that in general, calling any library function while running 
with nondefault trap settings is undefined behavior, unless the library was 
itself compiled with FENV_ACCESS ON.  There does not even appear to be any 
requirement for the C standard library functions to have been compiled with 
FENV_ACCESS ON, as far as I can see ...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53157/new/

https://reviews.llvm.org/D53157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

jyknight wrote:
> uweigand wrote:
> > jyknight wrote:
> > > Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
> > > InitPreprocessor alongside the rest of the SIZEOF macros?
> > > 
> > > And then use that to determine whether to add float128 to the union? This 
> > > change, as is, will break on any target which is i386 but doesn't define 
> > > __float128, e.g. i386-unknown-unknown.
> > > 
> > > The only additional target which will be modified with that (that is: the 
> > > only other target which has a float128 type, but for which max_align 
> > > isn't already 16) is systemz-*-linux.
> > > 
> > > But I think that's actually indicating a pre-existing bug in the SystemZ 
> > > config -- it's configured for a 16-byte long double, with 8-byte 
> > > alignment, but the ABI doc seems to call for 16-byte alignment. +Ulrich 
> > > for comment on that.
> > That's a bug in the ABI doc which we'll fix once we get around to releasing 
> > an updated version.
> > 
> > long double on SystemZ must be 8-byte aligned, which is the maximum 
> > alignment of all standard types on Z, and this was chosen because 
> > historically the ABI only guarantees an 8-byte stack alignment, so 16-byte 
> > aligned standard types would be awkward.
> Then perhaps it's a bug that `__alignof__(__float128)` returns 16 with 
> `-target systemz-linux`?
Huh, really __float128 should not be supported at all on SystemZ.  It's not 
supported with GCC either (GCC never provides __float128 on targets where long 
double is already IEEE-128).  (GCC does support the new _Float128 on SystemZ, 
but that's not yet supported by clang anywhere.)

If it were supported, I agree it should be an alias for long double, and also 
have an alignof of 8.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55057/new/

https://reviews.llvm.org/D55057



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In https://reviews.llvm.org/D53157#1305233, @kpn wrote:

> In https://reviews.llvm.org/D53157#1304347, @uweigand wrote:
>
> > But given that there is still infrastructure missing in the IR optimizers, 
> > I also think that at least in the first implementation, we probably should 
> > go with the original approach and just use constrained intrinsics 
> > everywhere in the function, and possibly add some function attribute that 
> > prevent any cross-inlining of functions built with constrained intrinsics 
> > with functions built with regular floating-point operations.
>
>
> Subtle. This last sentence seems to imply that cross-inlining should be 
> allowed when there are no regular floating point operations in the function 
> to be inlined. This makes sense due to, for example, the common use of tiny 
> functions just to retrieve a value. Do I interpret your statement correctly?


Sure, that would be an optimization.  Another potential optimization would be 
to **allow** inlining, but have the inliner automatically replace regular 
floating-point operations with constrained intrinsics in the target routine ... 
  But all of that is probably "stage2" after an initial implementation that 
just disables inlining.


https://reviews.llvm.org/D53157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

It seems this patch caused the SystemZ build bots to fail, they're now all 
running into assertion failures:

ICE cannot be evaluated!
UNREACHABLE executed at 
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/lib/AST/ExprConstant.cpp:11442!

See e.g. 
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/20645/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Abuiltins-systemz-zvector-error.c


Repository:
  rC Clang

https://reviews.llvm.org/D54355



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

OK, let me try to expand on my point 3 above, which appears to have confused 
everybody :-)

First, let's distinguish two separate requirements: those on floating-point 
operations that explicitly run in regions with non-default control modes, and 
those on floating-point operations that run outside such regions.  (Note that 
those regions are by definition a subset of the regions marked as FENV_ACCESS 
ON, but not necessarily coincide with them.)

For the first class of operations, there are various requirements to constrain 
optimization.  All of those are handled in the current design by simply 
representing those operations via constrained intrinsics.  This can be done 
simply by always using constrained intrinsics whenever we are inside any 
FENV_ACCESS ON region.  (My comments above were not intended to refer to those 
at all.)

Now, for the second class of operations, most of these requirements do not 
apply.  However, there is one critical requirement that still **does** apply: 
such operations may never be moved to **inside** a region with non-default 
control modes.  (Where it makes a difference, the operation can in principle be 
even moved inside a FENV_ACCESS ON region, as long as the control modes 
actually have not yet changed from the default modes.)

As @andrew.w.kaylor mentioned above, to fulfil this requirement if suffices to 
prevent moving any such floating-point operation across any statement that 
modifies the control modes (or inspects the exception flags).  There is only a 
small number of statements that do so directly (those would all be inline asm 
or platform-specific builtins), but the action can be hidden behind a function 
call as well.  This applies in particular to the related C library calls with 
well-known names (which the compiler could detect), but in addition any random 
function call might **indirectly** contain such a statement (which the compiler 
will not know, in general).  Since LLVM today will freely move normal 
floating-point operations across normal function calls, this is a problem that 
needs to be addressed.

Now, one way to do that would be to ensure that any floating-point operation 
outside FENV_ACCESS ON regions is **also** implemented via constrained 
intrinsics, as long as there is any FENV_ACCESS ON present in the function at 
all.  As @cameron.mcinally mentioned above, that would prevent reordering 
(across any function call, even) and therefore solve the problem.  However, I 
believe there were concerns whether we can reliably ensure that property in the 
presence of optimizations like inlining, in particular if LTO is also used.  
Even if this can be resolved, I believe there were additional concerns that 
this might overly constrain optimization and lead to suboptimal performance.

This finally gets to the intent of my point 3 above, where I was trying to see 
if there is any way we can do better, that is correctly handle floating-point 
operations outside FENV_ACCESS ON regions **without** having to turn them all 
into constrained intrinsics.  One way might be to add some new code motion 
barrier in the IR optimizers that would prevent movement of floating-point 
operations across the location of any FENV_ACCESS pragma.  But  -- and this is 
I think what @kpn referered to above -- those locations don't really correspond 
to anything in the IR that could serve as such a barrier.  My observation above 
was simply that if we want to go that way, it in fact isn't really necessary to 
put those barriers at exactly these places.  Instead, we can put the barriers 
at those places that will actually modify (or inspect) the control words -- 
which are either special operations we can explicitly recognize, or general 
function calls -- but only those calls where the call site was originally 
inside a FENV_ACCESS ON region (and was marked by the front-end as such).  This 
has the advantage that a function call site is a natural place to act as 
barrier -- in fact it already acts as such e.g. for global memory accesses. (It 
would also be easy to implement as barrier on the MI / back-end level.)  In 
addition, this approach would not lead to any performance penalty outside of 
FENV_ACCESS ON regions at all.

But given that there is still infrastructure missing in the IR optimizers, I 
also think that at least in the first implementation, we probably should go 
with the original approach and just use constrained intrinsics everywhere in 
the function, and possibly add some function attribute that prevent any 
cross-inlining of functions built with constrained intrinsics with functions 
built with regular floating-point operations.


https://reviews.llvm.org/D53157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In https://reviews.llvm.org/D53157#1303193, @andrew.w.kaylor wrote:

> I agree that it's preferable to re-use these existing options if possible. I 
> have some concerns that -ftrapping-math has a partial implementation in place 
> that doesn't seem to be well aligned with the way fast-math flags are 
> handled, so it might require some work to have that working as expected 
> without breaking existing users. In general though these seem like they 
> should do what we need.


So as far as I can see, the current implementation of -f(no-)trapping-math in 
LLVM is pretty much a stub.  The driver passes the option on to CC1, and in 
addition it disables -menable-unsafe-fp-math and -massociative-math if 
-fno-trapping-math is in effect (the latter may be a minor difference to GCC, 
but should be conservatively compatible).  The compiler itself does nothing 
except mark all functions with the no-trapping-math attribute if 
-fno-trapping-math is in effect.  LLVM itself completely ignores this attribute 
except on ARM, where it is used to set the ABI_FP_exceptions extended 
attribute.  (But since LLVM doesn't really implement the flag in actual 
codegen, setting the attribute to indicate the property seems a bit odd ...)

> Regarding GCC compatibility, I notice that GCC defaults to trapping math 
> being enabled and I don't think that's what we want with clang. It also seems 
> to imply something more than I think we need for constrained handling. For 
> example, the GCC documentation says that -fno-trapping-math "can result in 
> incorrect output for programs that depend on an exact implementation of IEEE 
> or ISO rules/specifications for math functions" so it sounds like maybe it 
> also implies (for GCC)  something like LLVM's "afn" fast math flag.
> 
> So if we are going to use these options, I think we need to have a discussion 
> about whether or not it's OK to diverge from GCC's interpretation of them.

The GCC documentation says:

> -fno-trapping-math
> 
> Compile code assuming that floating-point operations cannot generate 
> user-visible traps.  These traps include division by zero, overflow, 
> underflow, inexact result and invalid operation.  This option requires that 
> -fno-signaling-nans be in effect.  Setting this option may allow faster code 
> if one relies on ``non-stop'' IEEE arithmetic, for example.
> 
> This option should never be turned on by any -O option since it can result in 
> incorrect output for programs that depend on an exact implementation of IEEE 
> or ISO rules/specifications for math functions.
> 
> The default is -ftrapping-math.

As far as I can tell, this does not imply any **additional** differences except 
that -fno-trapping-math may rearrange floating-point operations to eliminate or 
introduce traps or changes to exception flags.  It is **those** changes 
themselves (not anything else) that can result in incorrect output for programs 
that depend on exact IEEE semantics, and therefore this option must not be 
enabled by default at any optimization level, as the second paragraph above 
states.

Note that in GCC, -fno-trapping-math is implied by -funsafe-math-optimizations, 
which in turn is implied by -ffast-math.   Those options all are documented 
using the same "incorrect output for programs that depend on exact IEEE 
semantics" clause: -funsafe-math-optimization is the catch-all for all such 
effects, but it is separated into -fno-signed-zeros, -fno-trapping-math, 
-fassociative-math, and -freciprocal-math that cover the distinct aspects of 
just how IEEE semantics might get violated.

We can of course still have a separate discussion on what the default should 
be.  But even if we choose a different default than GCC (as is already the case 
e.g. for the -ffp-contract option), I think it would be preferable for the two 
explicit options to behave in a compatible way.


https://reviews.llvm.org/D53157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

A couple of comments on the previous discussion:

1. Instead of defining a new command line option, I'd prefer to use the 
existing options -frounding-math and -ftrapping-math to set the default 
behavior of math operations w.r.t. rounding modes and exception status.  (For 
compatibility with GCC if nothing else.)
2. I also read the C standard to imply that it is a requirement of **user 
code** to reset the status flag to default before switching back to FENV_ACCESS 
OFF.  The fundamental characterization of the pragma says "The FENV_ACCESS 
pragma provides a means **to inform the implementation** when a program might 
access the floating-point environment to test floating-point status flags or 
run under non-default floating-point control modes."  There is no mention 
anywhere that using the pragma, on its own, will ever **change** those control 
modes.   The last sentence about "... the floating-point control modes have 
their default setting", while indeed a bit ambiguous, is still consistent with 
an interpretation that it is the responsibility of user code to ensure that 
state, there is no explicit statement that the implementation will do so.
3. I agree that we need to be careful about intermixing "normal" floating-point 
operations with strict ones.  However, I'm still not convinced that the pragma 
itself must be the scheduling barrier.  It seems to me that the compiler 
already knows where FP control flags are ever modified directly (this can only 
happen with intrinsics or the like), so the main issue is whether function 
calls need to be considered.  This is where the pragma comes in: in my mind, 
the primary difference between FENV_ACCESS ON and FENV_ACCESS OFF regions is 
that where the pragma is ON, function calls need to be considered (unless 
otherwise known for sure) to access FP control flags, while where the pragma is 
OFF, function calls can be considered to never touch FP control flags.  So the 
real scheduling barrier would be any **function call within a FENV_ACCESS ON 
region**.  Those would have to be marked by the front-end in the IR, presumably 
using a function attribute.  The common LLVM optimizers would then need to 
respect that scheduling barrier (here is where we likely still have an open 
issue, there doesn't appear to be any way to express that at the IR level for 
regular floating-point operations ...), and likewise the back-ends (but that 
looks straightforward: a back-end typically will model FP status as residing in 
a register or in a pseudo-memory slot, and those can simply be considered 
used/clobbered by function calls marked as within FENV_ACCESS ON regions).




https://reviews.llvm.org/D53157



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Looks like this test is failing on SystemZ since it was added, making all our 
build bots red:

  
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/CodeGenCXX/builtin_FUNCTION.cpp:9:11:
 error: CHECK: expected string not found in input
  // CHECK: @[[EMPTY_STR:.+]] = private unnamed_addr constant [1 x i8] 
zeroinitializer, align 1
^
  
/home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/tools/clang/test/CodeGenCXX/Output/builtin_FUNCTION.cpp.tmp.ll:1:1:
 note: scanning from here
  ; ModuleID = 
'/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/CodeGenCXX/builtin_FUNCTION.cpp'
  ^
  
/home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/tools/clang/test/CodeGenCXX/Output/builtin_FUNCTION.cpp.tmp.ll:8:1:
 note: possible intended match here
  @.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 2
  ^

The problem is that string constants are 2-aligned according to the SystemZ ABI 
(this is a bit different, but deliberate in order to allow computing their 
addresses with a LARL instruction).  This makes all the "align 1" checks fail.

Is this test deliberately verifying the alignment, or could this just be 
removed?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37035/new/

https://reviews.llvm.org/D37035



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

As of r361920, the SystemZ build bots are green again.   Thanks, Eric!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37035/new/

https://reviews.llvm.org/D37035



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63366: AMDGPU: Add GWS instruction builtins

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

I'm now getting test suite failures like this:

  
/home/uweigand/llvm/llvm-head/tools/clang/test/CodeGenOpenCL/builtins-amdgcn.cl:554:3:
 error: cannot compile this builtin function yet
__builtin_amdgcn_ds_gws_init(value, id);

Is this supposed to work?  I'm not seeing any LLVM support for this builtin 
anywhere ...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63366/new/

https://reviews.llvm.org/D63366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66795: [Mips] Use appropriate private label prefix based on Mips ABI

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

This makes sense to me (although we don't currently need the options parameter 
there).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66795/new/

https://reviews.llvm.org/D66795



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67763: [Clang FE] Recognize -mnop-mcount CL option

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

This looks good to me now.  Given that this patch implements an option only 
relevant on SystemZ, this should be OK.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67763/new/

https://reviews.llvm.org/D67763



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71049: Remove implicit conversion that promotes half to other larger precision types for fp classification builtins

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

This causes build bot failures on SystemZ due to a failed assertion in 
ConstantFP::getInfinity:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/28723/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Abuiltins.c


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71049/new/

https://reviews.llvm.org/D71049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand created this revision.
uweigand added reviewers: kpn, andrew.w.kaylor, craig.topper, cameron.mcinally, 
RKSimon, spatel, rjmccall, rsmith.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Update the IRBuilder to generate constrained FP comparisons in CreateFCmp when 
IsFPConstrained is true, similar to the other places in the IRBuilder.

Also, add a new CreateFCmpS to emit **signaling** FP comparisons, and use it in 
clang where comparisons are supposed to be signaling (currently, only when 
emitting code for the <, <=, >, >= operators).  Most other places are supposed 
to emit quiet comparisons, including the equality comparisons, the various 
builtins like isless, and uses of floating-point values in boolean contexts.  A 
few places that I haven't touched may need some extra thought (e.g. are 
comparisons implicitly generated to implement sanitizer checks supposed to be 
signaling?).

I've noticed two potential problems while implementing this:

- There is currently no way to add fast-math flags to a constrained FP 
comparison, since this is implemented as an intrinsic call that returns a 
boolean type, and FMF are only allowed for calls returning a floating-point 
type.  However, given the discussion around 
https://bugs.llvm.org/show_bug.cgi?id=42179, it seems that FCmp itself really 
shouldn't have any FMF either, so this is probably OK.

- Using builtins like __builtin_isless on a "float" type will implicitly 
convert the float argument to a double; apparently this is because the builtin 
is declared as having a variable argument list?   In any case, that means that 
even though a quiet comparison is generated, the semantics still isn't correct 
for quiet NaNs, as that implicit conversion will already signal an exception.  
This probably needs to be fixed, but I guess that can be done as a separate 
patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71467

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/fpconstrained-cmp.c
  llvm/include/llvm/IR/IRBuilder.h

Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -1161,6 +1161,18 @@
 return MetadataAsValue::get(Context, ExceptMDS);
   }
 
+  Value *getConstrainedFPPredicate(CmpInst::Predicate Predicate) {
+assert(CmpInst::isFPPredicate(Predicate) &&
+   Predicate != CmpInst::FCMP_FALSE &&
+   Predicate != CmpInst::FCMP_TRUE &&
+   "Invalid constrained FP comparison predicate!");
+
+StringRef PredicateStr = CmpInst::getPredicateName(Predicate);
+auto *PredicateMDS = MDString::get(Context, PredicateStr);
+
+return MetadataAsValue::get(Context, PredicateMDS);
+  }
+
 public:
   Value *CreateAdd(Value *LHS, Value *RHS, const Twine  = "",
bool HasNUW = false, bool HasNSW = false) {
@@ -2308,12 +2320,41 @@
 
   Value *CreateFCmp(CmpInst::Predicate P, Value *LHS, Value *RHS,
 const Twine  = "", MDNode *FPMathTag = nullptr) {
+if (IsFPConstrained)
+  return CreateConstrainedFPCmp(Intrinsic::experimental_constrained_fcmp,
+P, LHS, RHS, Name);
+
+if (auto *LC = dyn_cast(LHS))
+  if (auto *RC = dyn_cast(RHS))
+return Insert(Folder.CreateFCmp(P, LC, RC), Name);
+return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), Name);
+  }
+
+  Value *CreateFCmpS(CmpInst::Predicate P, Value *LHS, Value *RHS,
+ const Twine  = "", MDNode *FPMathTag = nullptr) {
+if (IsFPConstrained)
+  return CreateConstrainedFPCmp(Intrinsic::experimental_constrained_fcmps,
+P, LHS, RHS, Name);
+
 if (auto *LC = dyn_cast(LHS))
   if (auto *RC = dyn_cast(RHS))
 return Insert(Folder.CreateFCmp(P, LC, RC), Name);
 return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), Name);
   }
 
+  CallInst *CreateConstrainedFPCmp(
+  Intrinsic::ID ID, CmpInst::Predicate P, Value *L, Value *R,
+  const Twine  = "",
+  Optional Except = None) {
+Value *PredicateV = getConstrainedFPPredicate(P);
+Value *ExceptV = getConstrainedFPExcept(Except);
+
+CallInst *C = CreateIntrinsic(ID, {L->getType()},
+  {L, R, PredicateV, ExceptV}, nullptr, Name);
+setConstrainedFPCallAttr(C);
+return C;
+  }
+
   //======//
   // Instruction creation methods: Other Instructions
   //======//
Index: clang/test/CodeGen/fpconstrained-cmp.c
===
--- /dev/null
+++ clang/test/CodeGen/fpconstrained-cmp.c
@@ -0,0 +1,151 @@
+// RUN: %clang_cc1 -ffp-exception-behavior=ignore -emit-llvm -o - %s | FileCheck %s 

[PATCH] D71408: [lit] Remove lit's REQUIRES-ANY directive

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

It looks like this caused build bot failures:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/28928/steps/ninja%20check%201/logs/FAIL%3A%20lit%3A%3A%20shtest-format.py

  
/home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/utils/lit/tests/shtest-format.py:52:10:
 error: CHECK: expected string not found in input
  # CHECK: UNSUPPORTED: shtest-format :: requires-any-missing.txt
   ^
  :71:33: note: scanning from here
  PASS: shtest-format :: pass.txt (8 of 22)
  ^
  :72:1: note: possible intended match here
  UNSUPPORTED: shtest-format :: requires-missing.txt (9 of 22)
  ^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71408/new/

https://reviews.llvm.org/D71408



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand updated this revision to Diff 234096.
uweigand added a comment.

Added float (f32) test cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/fpconstrained-cmp-double.c
  clang/test/CodeGen/fpconstrained-cmp-float.c
  llvm/include/llvm/IR/IRBuilder.h

Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -1161,6 +1161,18 @@
 return MetadataAsValue::get(Context, ExceptMDS);
   }
 
+  Value *getConstrainedFPPredicate(CmpInst::Predicate Predicate) {
+assert(CmpInst::isFPPredicate(Predicate) &&
+   Predicate != CmpInst::FCMP_FALSE &&
+   Predicate != CmpInst::FCMP_TRUE &&
+   "Invalid constrained FP comparison predicate!");
+
+StringRef PredicateStr = CmpInst::getPredicateName(Predicate);
+auto *PredicateMDS = MDString::get(Context, PredicateStr);
+
+return MetadataAsValue::get(Context, PredicateMDS);
+  }
+
 public:
   Value *CreateAdd(Value *LHS, Value *RHS, const Twine  = "",
bool HasNUW = false, bool HasNSW = false) {
@@ -2308,12 +2320,41 @@
 
   Value *CreateFCmp(CmpInst::Predicate P, Value *LHS, Value *RHS,
 const Twine  = "", MDNode *FPMathTag = nullptr) {
+if (IsFPConstrained)
+  return CreateConstrainedFPCmp(Intrinsic::experimental_constrained_fcmp,
+P, LHS, RHS, Name);
+
+if (auto *LC = dyn_cast(LHS))
+  if (auto *RC = dyn_cast(RHS))
+return Insert(Folder.CreateFCmp(P, LC, RC), Name);
+return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), Name);
+  }
+
+  Value *CreateFCmpS(CmpInst::Predicate P, Value *LHS, Value *RHS,
+ const Twine  = "", MDNode *FPMathTag = nullptr) {
+if (IsFPConstrained)
+  return CreateConstrainedFPCmp(Intrinsic::experimental_constrained_fcmps,
+P, LHS, RHS, Name);
+
 if (auto *LC = dyn_cast(LHS))
   if (auto *RC = dyn_cast(RHS))
 return Insert(Folder.CreateFCmp(P, LC, RC), Name);
 return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), Name);
   }
 
+  CallInst *CreateConstrainedFPCmp(
+  Intrinsic::ID ID, CmpInst::Predicate P, Value *L, Value *R,
+  const Twine  = "",
+  Optional Except = None) {
+Value *PredicateV = getConstrainedFPPredicate(P);
+Value *ExceptV = getConstrainedFPExcept(Except);
+
+CallInst *C = CreateIntrinsic(ID, {L->getType()},
+  {L, R, PredicateV, ExceptV}, nullptr, Name);
+setConstrainedFPCallAttr(C);
+return C;
+  }
+
   //======//
   // Instruction creation methods: Other Instructions
   //======//
Index: clang/test/CodeGen/fpconstrained-cmp-float.c
===
--- /dev/null
+++ clang/test/CodeGen/fpconstrained-cmp-float.c
@@ -0,0 +1,151 @@
+// RUN: %clang_cc1 -ffp-exception-behavior=ignore -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=FCMP
+// RUN: %clang_cc1 -ffp-exception-behavior=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=EXCEPT
+// RUN: %clang_cc1 -ffp-exception-behavior=maytrap -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=MAYTRAP
+// RUN: %clang_cc1 -frounding-math -ffp-exception-behavior=ignore -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=IGNORE
+// RUN: %clang_cc1 -frounding-math -ffp-exception-behavior=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=EXCEPT
+// RUN: %clang_cc1 -frounding-math -ffp-exception-behavior=maytrap -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=MAYTRAP
+
+_Bool QuietEqual(float f1, float f2) {
+  // CHECK-LABEL: define {{.*}}i1 @QuietEqual(float %f1, float %f2)
+
+  // FCMP: fcmp oeq float %{{.*}}, %{{.*}}
+  // IGNORE: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, float %{{.*}}, metadata !"oeq", metadata !"fpexcept.ignore")
+  // EXCEPT: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, float %{{.*}}, metadata !"oeq", metadata !"fpexcept.strict")
+  // MAYTRAP: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, float %{{.*}}, metadata !"oeq", metadata !"fpexcept.maytrap")
+  return f1 == f2;
+
+  // CHECK: ret
+}
+
+_Bool QuietNotEqual(float f1, float f2) {
+  // CHECK-LABEL: define {{.*}}i1 @QuietNotEqual(float %f1, float %f2)
+
+  // FCMP: fcmp une float %{{.*}}, %{{.*}}
+  // IGNORE: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, float %{{.*}}, metadata !"une", metadata !"fpexcept.ignore")
+  // EXCEPT: call i1 

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D71467#1785943 , @erichkeane wrote:

> I did the compare operators that didn't work right, and will do a separate 
> patch for the fp-classification type ones: 
> f02d6dd6c7afc08f871a623c0411f2d77ed6acf8 
> 


Thanks!  Now I'm getting the correct output for the float test cases as well, 
and I've added them to the patch.

As to fp-classification, I think there is an additional complication here:  
according to IEEE and the proposed C2x standard, these builtins should 
**never** raise any exception, not even when receiving a signaling NaN as 
input.  Strictly speaking, this means that they cannot possibly be implemented 
in terms of any comparison operation.

Now, on SystemZ (and many other platforms, I think) there are in fact 
specialized instructions that will implement the required semantics without 
raising any exceptions, but there seems to be no way to represent those at the 
LLVM IR level.  We'll probably need some extensions here (some new IR-level 
builtins?) ...

(But I'd say that problem is unrelated to this patch, so I'd prefer to decouple 
that problem from the question of whether **this** patch is the right solution 
for comparisons.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D71467#1786192 , @erichkeane wrote:

> __builtin_fpclassify/isfinite/isinf/isinf_sign/isnan/isnormal/signbit are all 
> implemented the same as the OTHER ones, except there is a strange fixup step 
> in SEMA that removes the float->double cast.  It is IMO the wrong way to do 
> it.
>
> I don't think it would modify the IR at all or the AST, but I'm also working 
> on removing that hack (which is what I meant by the fp-classification type 
> ones).
>
> I hope the work I've done already is sufficient to unblock this patch.


Yes, this patch is no longer blocked, thanks!

What I was trying to say is that there is a fundamental difference between the 
comparison builtins like isless, isgreater, etc. and the classification 
builtins like isinf, isnan, etc.

The former **should** result in comparison instructions being generated, the 
only difference between the builtin and a regular "<" operator is that the 
builtin emits a quiet compare while the operator emits a signaling compare in 
strict mode.

However, the latter (classification macros) should not actually emit **any** 
comparison instructions in strict mode, because the classification macros may 
never trap, but all comparison instructions do.  So the basic idea of 
implementing e.g. isinf(x) as "fabs(x) == infinity"  (like the comment in 
CGBuiltin.cpp currently says) is fundamentally wrong in strict mode.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71669: [Clang FE, SystemZ] Don't add "true" value for the "mnop-mcount" attribute.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71669/new/

https://reviews.llvm.org/D71669



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Ping?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72722: [FPEnv] [SystemZ] Platform-specific builtin constrained FP enablement

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72722/new/

https://reviews.llvm.org/D72722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72906: [X86] Improve X86 cmpps/cmppd/cmpss/cmpsd intrinsics with strictfp

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D72906#1837905 , @craig.topper 
wrote:

> In D72906#1826849 , @uweigand wrote:
>
> > In D72906#1826122 , @craig.topper 
> > wrote:
> >
> > > In D72906#1826061 , @uweigand 
> > > wrote:
> > >
> > > > > The constrained fcmp intrinsics don't allow the TRUE/FALSE predicates.
> > > >
> > > > Hmm, maybe they should then?   The only reason I didn't add them 
> > > > initially was that I wasn't sure they were useful for anything; if they 
> > > > are, it should be straightforward to add them back.
> > >
> > >
> > > What would we lower it to on a target that doesn’t support it natively?
> >
> >
> > Any supported compare (quiet or signaling as appropriate, just so we get 
> > the correct exceptions), and then ignore the result (and use true/false 
> > constant result instead)?
>
>
> Sure. Is that something we want to force all targets to have to implement 
> just to handle this case for X86? Unless we can come up with a generic DAG 
> combine to pick a valid condition alternate so that the lowering code for 
> each target doesn't have to deal with it.


Hmm, OK.  Given that this X86-specific builtin code seems to be the only place 
in clang where FCMP_TRUE/FCMP_FALSE can ever get emitted anyway, I guess you 
might as well implement the workaround (use some other compare to get the 
exception) right there, just for X86.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72906/new/

https://reviews.llvm.org/D72906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74146: [SytemZ] Disable vector ABI when using option -march=arch[8|9|10]

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74146/new/

https://reviews.llvm.org/D74146



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D72675#1829866 , @hfinkel wrote:

> > I'm not sure whether this is deliberate (but it seems weird) or just a bug. 
> > I can ask the GCC developers ...
>
> Please do. If there's a rationale, we should know.


Sorry for the delay ... I've raised that question on the GCC list, and the 
opinion seems to be that the current behavior is indeed a bug -- however 
there's still discussion ongoing on what the proper fix ought to be.  I'll 
report back once we've agreed on a solution on the GCC side ...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72675/new/

https://reviews.llvm.org/D72675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D71467#1820589 , @rjmccall wrote:

> I think I have a slight preference for the second option, where there's a 
> single method that does all the work for the two cases.


OK, now checked in as 870137d 
 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D71467#1817939 , @rjmccall wrote:

> Is this approach going to work with scope-local strictness?  We need a way to 
> do a comparison that has the non-strict properties but appears in a function 
> that enables strictness elsewhere.


Well, just like for all the other FP builder methods, you can use the 
setIsFPConstrained method on the builder object to switch between strict and 
non-strict mode.   Does this not suffice, or is there anything particular about 
the comparisons that would require anything extra?

> Please document the difference between these two methods.

OK, checked in header file comments as 6aca3e8 
.

> Can you make a helper method for the common code in the non-constrained paths 
> here?

Would you prefer something like

  private:
Value *CreateFCmpHelper(CmpInst::Predicate P, Value *LHS, Value *RHS,
const Twine , MDNode *FPMathTag) {
  if (auto *LC = dyn_cast(LHS))
if (auto *RC = dyn_cast(RHS))
  return Insert(Folder.CreateFCmp(P, LC, RC), Name);
  return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), 
Name);
}
  
  public:
Value *CreateFCmp(CmpInst::Predicate P, Value *LHS, Value *RHS,
  const Twine  = "", MDNode *FPMathTag = nullptr) {
  if (IsFPConstrained)
return CreateConstrainedFPCmp(Intrinsic::experimental_constrained_fcmp,
  P, LHS, RHS, Name);
  
  return CreateFCmpHelper(P, LHS, RHS, Name, FPMathTag);
}
[...]

or rather something like:

  private:
Value *CreateFCmpHelper(CmpInst::Predicate P, Value *LHS, Value *RHS,
bool IsSignaling, const Twine , MDNode 
*FPMathTag) {
  if (IsFPConstrained)
return CreateConstrainedFPCmp(IsSignaling ? 
Intrinsic::experimental_constrained_fcmps
  : 
Intrinsic::experimental_constrained_fcmp,
  P, LHS, RHS, Name);
  
  if (auto *LC = dyn_cast(LHS))
if (auto *RC = dyn_cast(RHS))
  return Insert(Folder.CreateFCmp(P, LC, RC), Name);
  return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), 
Name);
}
  
  public:
Value *CreateFCmp(CmpInst::Predicate P, Value *LHS, Value *RHS,
  const Twine  = "", MDNode *FPMathTag = nullptr) {
  return CreateFCmpHelper(P, LHS, RHS, false, Name, FPMathTag);
}
[...]

or maybe simply have CreateFCmpS call CreateFCmp directly in the non-strict 
case?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72722: [FPEnv] [SystemZ] Platform-specific builtin constrained FP enablement

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

A few comments (see inline) -- otherwise this looks good to me, thanks!




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13354
+  return Builder.CreateFNeg(Builder.CreateConstrainedFPCall(F, {X, Y,  
Z}), "neg");
+
+} else {

Spurious empty line.



Comment at: clang/test/CodeGen/builtins-systemz-vector2-constrained.c:25
+  // CHECK: [[NEG:%[^ ]+]] = fneg <2 x double> {{.*}}
+  // CHECK:  [[RES:%[^ ]+]] = call <2 x double> 
@llvm.experimental.constrained.fma.v2f64(<2 x double> %{{.*}}, <2 x double> 
%{{.*}}, <2 x double> [[NEG]], metadata !{{.*}}, metadata !{{.*}})
+  // CHECK: fneg <2 x double> [[RES]]

Why is it that this one has metadata nodes and all the others do not?   Do they 
really not have metadata (why?) or are you just not checking for them?



Comment at: clang/test/CodeGen/builtins-systemz-zvector2-constrained.c:12
+// FIXME: The lack of constant folding for strict fp breaks this test.
+// See label FIXME-CHECK-ASM.
+

This was caused by unnecessary implicit conversions in the vecintrin.h code, 
I've now checked in cebba7c to remove those.   Can you rebase?  This FIXME 
should no longer be necessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72722/new/

https://reviews.llvm.org/D72722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72722: [FPEnv] [SystemZ] Platform-specific builtin constrained FP enablement

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

> What are the semantics of vfnmadb with respect to when it rounds vs the 
> negation?

Hmm, interesting.  The z/Architecture Principles of Operation states:

> The results for each element of VECTOR FP NEGA-
>  TIVE MULTIPLY AND ADD and VECTOR FP NEGA-
>  TIVE MULTIPLY AND SUBTRACT are the same as
>  for VECTOR FP MULTIPLY AND ADD and VECTOR
>  FP MULTIPLY AND SUBTRACT, respectively, except
>  the sign bit of numeric results are inverted. When the
>  result is a NaN it’s sign bit is unchanged.

So as far as rounding is concerned, the vfnmadb should have the exact same 
result as a vfmadb ; vflcdb sequence we'd get from a fma/fneg.

I hadn't realized the NaN special treatment before.  This makes the result 
actually different in that case.  OTOH I guess IEEE only says the result has to 
some NaN, not particularly **which** NaN, so that's probably also OK.  (In any 
case it's unrelated to strict FP.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72722/new/

https://reviews.llvm.org/D72722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72906: [X86] Improve X86 cmpps/cmppd/cmpss/cmpsd intrinsics with strictfp

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

> The constrained fcmp intrinsics don't allow the TRUE/FALSE predicates.

Hmm, maybe they should then?   The only reason I didn't add them initially was 
that I wasn't sure they were useful for anything; if they are, it should be 
straightforward to add them back.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72906/new/

https://reviews.llvm.org/D72906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72906: [X86] Improve X86 cmpps/cmppd/cmpss/cmpsd intrinsics with strictfp

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D72906#1826122 , @craig.topper 
wrote:

> In D72906#1826061 , @uweigand wrote:
>
> > > The constrained fcmp intrinsics don't allow the TRUE/FALSE predicates.
> >
> > Hmm, maybe they should then?   The only reason I didn't add them initially 
> > was that I wasn't sure they were useful for anything; if they are, it 
> > should be straightforward to add them back.
>
>
> What would we lower it to on a target that doesn’t support it natively?


Any supported compare (quiet or signaling as appropriate, just so we get the 
correct exceptions), and then ignore the result (and use true/false constant 
result instead)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72906/new/

https://reviews.llvm.org/D72906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

I've had a quick look at GCC, and it seems there's a couple of different issues.

First of all, `-ffast-math` is a "collective" flag that has no separate meaning 
except setting a bunch of other flags.  Currently, these flags are:  
`-fno-math-errno`, ` -funsafe-math-optimizations` (which is itself a collective 
flag enabling `-fno-signed-zeros`, `-fno-trapping-math`, `-fassociative-math`, 
and `-freciprocal-math`),  `-ffinite-math-only`, `-fno-rounding-math`, 
`-fno-signaling-nans`, `-fcx-limited-range`, and  `-fexcess-precision=fast`.

When deciding whether to define `__FAST_MATH__`, GCC will not specifically look 
at whether or not the -ffast-math option itself was given, but whether the 
status of those other flags matches what would have been set by -ffast-math.  
However, it does not include all of the flags; currently only `-fmath-errno`, 
`-funsafe-math-optimizations`, `-fsigned-zeros`, `-ftrapping-math`, 
`-ffinite-math-only`,  and `-fexcess-precision` are checked, while 
`-fassociative-math`, `-freciprocal-math`, `-frounding-math`, 
`-fsignaling-nans`, and `-fcx-limited-range` are ignored.

I'm not sure whether this is deliberate (but it seems weird) or just a bug.  I 
can ask the GCC developers ...

A separate question is the interaction of `-ffast-math` with `-ffp-contract=`.  
Currently, there is no such interaction whatsoever in GCC: `-ffast-math` does 
not imply any particular `-ffp-contract=` setting, and vice versa the 
`-ffp-contract=` setting is not considered at all when defining 
`__FAST_MATH__`.  This seems at least internally consistent.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72675/new/

https://reviews.llvm.org/D72675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Ping again.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

[Ulrich Weigand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG76e9c2a9870e: [FPEnv] Generate constrained FP comparisons 
from clang (authored by uweigand).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71467/new/

https://reviews.llvm.org/D71467

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/fpconstrained-cmp-double.c
  clang/test/CodeGen/fpconstrained-cmp-float.c
  llvm/include/llvm/IR/IRBuilder.h

Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -1195,6 +1195,18 @@
 return MetadataAsValue::get(Context, ExceptMDS);
   }
 
+  Value *getConstrainedFPPredicate(CmpInst::Predicate Predicate) {
+assert(CmpInst::isFPPredicate(Predicate) &&
+   Predicate != CmpInst::FCMP_FALSE &&
+   Predicate != CmpInst::FCMP_TRUE &&
+   "Invalid constrained FP comparison predicate!");
+
+StringRef PredicateStr = CmpInst::getPredicateName(Predicate);
+auto *PredicateMDS = MDString::get(Context, PredicateStr);
+
+return MetadataAsValue::get(Context, PredicateMDS);
+  }
+
 public:
   Value *CreateAdd(Value *LHS, Value *RHS, const Twine  = "",
bool HasNUW = false, bool HasNSW = false) {
@@ -2351,12 +2363,41 @@
 
   Value *CreateFCmp(CmpInst::Predicate P, Value *LHS, Value *RHS,
 const Twine  = "", MDNode *FPMathTag = nullptr) {
+if (IsFPConstrained)
+  return CreateConstrainedFPCmp(Intrinsic::experimental_constrained_fcmp,
+P, LHS, RHS, Name);
+
+if (auto *LC = dyn_cast(LHS))
+  if (auto *RC = dyn_cast(RHS))
+return Insert(Folder.CreateFCmp(P, LC, RC), Name);
+return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), Name);
+  }
+
+  Value *CreateFCmpS(CmpInst::Predicate P, Value *LHS, Value *RHS,
+ const Twine  = "", MDNode *FPMathTag = nullptr) {
+if (IsFPConstrained)
+  return CreateConstrainedFPCmp(Intrinsic::experimental_constrained_fcmps,
+P, LHS, RHS, Name);
+
 if (auto *LC = dyn_cast(LHS))
   if (auto *RC = dyn_cast(RHS))
 return Insert(Folder.CreateFCmp(P, LC, RC), Name);
 return Insert(setFPAttrs(new FCmpInst(P, LHS, RHS), FPMathTag, FMF), Name);
   }
 
+  CallInst *CreateConstrainedFPCmp(
+  Intrinsic::ID ID, CmpInst::Predicate P, Value *L, Value *R,
+  const Twine  = "",
+  Optional Except = None) {
+Value *PredicateV = getConstrainedFPPredicate(P);
+Value *ExceptV = getConstrainedFPExcept(Except);
+
+CallInst *C = CreateIntrinsic(ID, {L->getType()},
+  {L, R, PredicateV, ExceptV}, nullptr, Name);
+setConstrainedFPCallAttr(C);
+return C;
+  }
+
   //======//
   // Instruction creation methods: Other Instructions
   //======//
Index: clang/test/CodeGen/fpconstrained-cmp-float.c
===
--- /dev/null
+++ clang/test/CodeGen/fpconstrained-cmp-float.c
@@ -0,0 +1,151 @@
+// RUN: %clang_cc1 -ffp-exception-behavior=ignore -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=FCMP
+// RUN: %clang_cc1 -ffp-exception-behavior=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=EXCEPT
+// RUN: %clang_cc1 -ffp-exception-behavior=maytrap -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=MAYTRAP
+// RUN: %clang_cc1 -frounding-math -ffp-exception-behavior=ignore -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=IGNORE
+// RUN: %clang_cc1 -frounding-math -ffp-exception-behavior=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=EXCEPT
+// RUN: %clang_cc1 -frounding-math -ffp-exception-behavior=maytrap -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK -check-prefix=MAYTRAP
+
+_Bool QuietEqual(float f1, float f2) {
+  // CHECK-LABEL: define {{.*}}i1 @QuietEqual(float %f1, float %f2)
+
+  // FCMP: fcmp oeq float %{{.*}}, %{{.*}}
+  // IGNORE: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, float %{{.*}}, metadata !"oeq", metadata !"fpexcept.ignore")
+  // EXCEPT: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, float %{{.*}}, metadata !"oeq", metadata !"fpexcept.strict")
+  // MAYTRAP: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, float %{{.*}}, metadata !"oeq", metadata !"fpexcept.maytrap")
+  return f1 == f2;
+
+  // CHECK: ret
+}
+
+_Bool QuietNotEqual(float f1, float f2) {
+  // CHECK-LABEL: define {{.*}}i1 @QuietNotEqual(float %f1, float %f2)
+
+  // FCMP: fcmp une float %{{.*}}, %{{.*}}
+  // IGNORE: call i1 @llvm.experimental.constrained.fcmp.f32(float %{{.*}}, 

[PATCH] D71854: [SystemZ] Use FNeg in s390x clang builtins

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71854/new/

https://reviews.llvm.org/D71854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71854: [SystemZ] Use FNeg in s390x clang builtins

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand requested changes to this revision.
uweigand added a comment.
This revision now requires changes to proceed.

This also needs updating the test cases that are testing for the old behavior:

Failing Tests (4):

  Clang :: CodeGen/builtins-systemz-vector.c
  Clang :: CodeGen/builtins-systemz-vector2.c
  Clang :: CodeGen/builtins-systemz-zvector.c
  Clang :: CodeGen/builtins-systemz-zvector2.c


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71854/new/

https://reviews.llvm.org/D71854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71854: [SystemZ] Use FNeg in s390x clang builtins

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Otherwise this LGTM.   Thanks for taking care of those!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71854/new/

https://reviews.llvm.org/D71854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75914: systemz: allow configuring default CLANG_SYSTEMZ_ARCH

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Thanks for working on this, @thakis !


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75914/new/

https://reviews.llvm.org/D75914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75914: systemz: allow configuring default CLANG_SYSTEMZ_ARCH

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Ah, good point.  Dimitry, can you prepare an updated patch to implement Jonas' 
suggestion?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75914/new/

https://reviews.llvm.org/D75914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75914: systemz: allow configuring default CLANG_SYSTEMZ_ARCH

[Ulrich Weigand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9c9d88d8b1bb: [SystemZ] Allow configuring default 
CLANG_SYSTEMZ_ARCH (authored by uweigand).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75914/new/

https://reviews.llvm.org/D75914

Files:
  clang/CMakeLists.txt
  clang/lib/Driver/ToolChains/Arch/SystemZ.cpp


Index: clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
===
--- clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
+++ clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
@@ -47,7 +47,7 @@
 
 return std::string(CPUName);
   }
-  return "z10";
+  return CLANG_SYSTEMZ_DEFAULT_ARCH;
 }
 
 void systemz::getSystemZTargetFeatures(const Driver , const ArgList ,
Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -306,6 +306,10 @@
 "Default architecture for OpenMP offloading to Nvidia GPUs." FORCE)
 endif()
 
+set(CLANG_SYSTEMZ_DEFAULT_ARCH "z10" CACHE STRING
+  "SystemZ Default Arch")
+add_definitions( -DCLANG_SYSTEMZ_DEFAULT_ARCH="${CLANG_SYSTEMZ_DEFAULT_ARCH}")
+
 set(CLANG_VENDOR ${PACKAGE_VENDOR} CACHE STRING
   "Vendor-specific text for showing with version information.")
 


Index: clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
===
--- clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
+++ clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
@@ -47,7 +47,7 @@
 
 return std::string(CPUName);
   }
-  return "z10";
+  return CLANG_SYSTEMZ_DEFAULT_ARCH;
 }
 
 void systemz::getSystemZTargetFeatures(const Driver , const ArgList ,
Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -306,6 +306,10 @@
 "Default architecture for OpenMP offloading to Nvidia GPUs." FORCE)
 endif()
 
+set(CLANG_SYSTEMZ_DEFAULT_ARCH "z10" CACHE STRING
+  "SystemZ Default Arch")
+add_definitions( -DCLANG_SYSTEMZ_DEFAULT_ARCH="${CLANG_SYSTEMZ_DEFAULT_ARCH}")
+
 set(CLANG_VENDOR ${PACKAGE_VENDOR} CACHE STRING
   "Vendor-specific text for showing with version information.")
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87279: [clang] Fix handling of physical registers in inline assembly operands.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D87279#2334510 , @jonpa wrote:

> The problem seems to be with a tied earlyclobber operand:
>
>   asm("" : "+"(a));
>
> Per the comment in InlineAsm::ConstraintInfo::Parse(), only output can be 
> earlyclobber.
>
> I am not sure if the earlyclobber ('&') should with this patch be removed 
> from the added use of the register, or if this has to for some reason really 
> be tied to the def?

So we have a read/write tied earlyclobber operand that is also tied to a phyreg 
via an asm register declaration?   That means that: the physreg is used as 
input, it is also used as output, and it is written to before all inputs are 
read (i.e. no *other* input may share the same register, even if it would 
otherwise hold the same value).

I believe there is no way to represent the effect of a read/write tied 
earlyclobber in any other way.  E.g. trying to use separate inputs/outputs (one 
with "=", one with "r") does not work since the earlyclobber on the output 
would prevent the same register to be used for the input, but it *has* to be 
the same register ...

On on other hand, in this special case the original problem cannot actually 
occur, because there cannot be any other input using the same physreg (that's 
what the earlyclobber ensures), so maybe be just don't do that transformation 
if an earlyclobber is present?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87279/new/

https://reviews.llvm.org/D87279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand updated this revision to Diff 276650.
uweigand added a comment.

Drop AllowNoUniqueAddress parameter; address review comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/systemz-abi.cpp


Index: clang/test/CodeGen/systemz-abi.cpp
===
--- clang/test/CodeGen/systemz-abi.cpp
+++ clang/test/CodeGen/systemz-abi.cpp
@@ -23,3 +23,37 @@
 // CHECK-LABEL: define void 
@_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret 
align 4 %{{.*}}, float %{{.*}})
 // SOFT-FLOAT-LABEL:  define void 
@_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret 
align 4 %{{.*}}, i32 %{{.*}})
 
+
+// A field member of empty class type in C++ makes the record nonhomogeneous,
+// unless it is marked as [[no_unique_address]].  This does not apply to 
arrays.
+struct empty { };
+struct agg_nofloat_empty { float a; empty dummy; };
+struct agg_nofloat_empty pass_agg_nofloat_empty(struct agg_nofloat_empty arg) 
{ return arg; }
+// CHECK-LABEL: define void 
@_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* 
noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void 
@_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* 
noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct agg_float_empty { float a; [[no_unique_address]] empty dummy; };
+struct agg_float_empty pass_agg_float_empty(struct agg_float_empty arg) { 
return arg; }
+// CHECK-LABEL: define void 
@_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias 
sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void 
@_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias 
sret align 4 %{{.*}}, i32 %{{.*}})
+struct agg_nofloat_emptyarray { float a; [[no_unique_address]] empty dummy[3]; 
};
+struct agg_nofloat_emptyarray pass_agg_nofloat_emptyarray(struct 
agg_nofloat_emptyarray arg) { return arg; }
+// CHECK-LABEL: define void 
@_Z27pass_agg_nofloat_emptyarray22agg_nofloat_emptyarray(%struct.agg_nofloat_emptyarray*
 noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void 
@_Z27pass_agg_nofloat_emptyarray22agg_nofloat_emptyarray(%struct.agg_nofloat_emptyarray*
 noalias sret align 4 %{{.*}}, i64 %{{.*}})
+
+// And likewise for members of base classes.
+struct noemptybase { empty dummy; };
+struct agg_nofloat_emptybase : noemptybase { float a; };
+struct agg_nofloat_emptybase pass_agg_nofloat_emptybase(struct 
agg_nofloat_emptybase arg) { return arg; }
+// CHECK-LABEL: define void 
@_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase*
 noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void 
@_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase*
 noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct emptybase { [[no_unique_address]] empty dummy; };
+struct agg_float_emptybase : emptybase { float a; };
+struct agg_float_emptybase pass_agg_float_emptybase(struct agg_float_emptybase 
arg) { return arg; }
+// CHECK-LABEL: define void 
@_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* 
noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void 
@_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* 
noalias sret align 4 %{{.*}}, i32 %{{.*}})
+struct noemptybasearray { [[no_unique_address]] empty dummy[3]; };
+struct agg_nofloat_emptybasearray : noemptybasearray { float a; };
+struct agg_nofloat_emptybasearray pass_agg_nofloat_emptybasearray(struct 
agg_nofloat_emptybasearray arg) { return arg; }
+// CHECK-LABEL: define void 
@_Z31pass_agg_nofloat_emptybasearray26agg_nofloat_emptybasearray(%struct.agg_nofloat_emptybasearray*
 noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void 
@_Z31pass_agg_nofloat_emptybasearray26agg_nofloat_emptybasearray(%struct.agg_nofloat_emptybasearray*
 noalias sret align 4 %{{.*}}, i64 %{{.*}})
+
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -499,11 +499,15 @@
 
   // Constant arrays of empty records count as empty, strip them off.
   // Constant arrays of zero length always count as empty.
+  bool WasArray = false;
   if (AllowArrays)
 while (const ConstantArrayType *AT = Context.getAsConstantArrayType(FT)) {
   if (AT->getSize() == 0)
 return true;
   FT = AT->getElementType();
+  // The [[no_unique_address]] special case below does not apply to
+  // arrays of C++ empty records, so we need to remember this fact.
+  WasArray = true;
 }
 
   const RecordType *RT = FT->getAs();
@@ -514,7 +518,14 @@
   //
   // FIXME: We should use a predicate for whether this 

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand marked 6 inline comments as done.
uweigand added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:521
+  // [[no_unique_address]] attribute (since C++20).  Those do count
+  // as empty according to the Itanium ABI.  This property is currently
+  // only respected if the AllowNoUniqueAddr parameter is true.

hubert.reinterpretcast wrote:
> This check is being done after removal of the array types by `AllowArrays`, 
> so this code is also conferring the property of being empty to arrays. It 
> seems GCC erroneously does the same for base class fields (but not for direct 
> members).
> 
> ```
> struct Empty {};
> 
> struct A {
>   Empty emp [[no_unique_address]][3];
> };
> 
> struct B : A {
>   float f;
> };
> 
> struct C {
>   Empty emp [[no_unique_address]][3];
>   float f;
> };
> 
> extern char szb[sizeof(B)];
> extern char szb[sizeof(float)]; // GCC likes this
> extern char szc[sizeof(C)];
> extern char szc[sizeof(float)]; // GCC does not like this
> ```
> 
> Compiler Explorer link: https://godbolt.org/z/NFuca9
This version should fix clang; I agree that GCC still gets this wrong.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:524
+  if (isa(RT->getDecl()) &&
+  !(AllowNoUniqueAddr && FD->hasAttr()))
 return false;

efriedma wrote:
> Does this do the right thing with a field that's an array of empty classes?
You're right.  As Hubert notes, arrays of empty classes do not count as empty.  
This version should fix the problem.  



Comment at: clang/lib/CodeGen/TargetInfo.cpp:7245
   // do count.  So do anonymous bitfields that aren't zero-sized.
-  if (getContext().getLangOpts().CPlusPlus &&
-  FD->isZeroLengthBitField(getContext()))
-continue;
+  if (getContext().getLangOpts().CPlusPlus) {
+if (FD->isZeroLengthBitField(getContext()))

efriedma wrote:
> Only loosely relevant to this patch, but checking getLangOpts().CPlusPlus 
> here seems weird; doesn't that break calling functions defined in C from C++ 
> code?
I agree that this difference between C and C++ is weird, but it does match the 
behavior of GCC.  (Which is itself weird, but a long-standing accident that we 
probably cannot fix without breaking existing code at this point.)

Now, you bring up an interesting point: When C++ code calls a function defined 
in C code, the C++ part would have to refer to an `extern "C"` declaration.  
The correct thing to do would probably be to handle those according to the C 
ABI rules, not the C++ rules, in this case where the two differ.  But currently 
GCC doesn't do that either.  (But since that would be broken anyway, I think we 
**can** fix that.)  In any case, I agree that this is really a separate 
problem, distinct from this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand updated this revision to Diff 276414.
uweigand added a comment.

Handle array of empty records correctly.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/systemz-abi.cpp

Index: clang/test/CodeGen/systemz-abi.cpp
===
--- clang/test/CodeGen/systemz-abi.cpp
+++ clang/test/CodeGen/systemz-abi.cpp
@@ -23,3 +23,37 @@
 // CHECK-LABEL: define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, float %{{.*}})
 // SOFT-FLOAT-LABEL:  define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, i32 %{{.*}})
 
+
+// A field member of empty class type in C++ makes the record nonhomogeneous,
+// unless it is marked as [[no_unique_address]].  This does not apply to arrays.
+struct empty { };
+struct agg_nofloat_empty { float a; empty dummy; };
+struct agg_nofloat_empty pass_agg_nofloat_empty(struct agg_nofloat_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct agg_float_empty { float a; [[no_unique_address]] empty dummy; };
+struct agg_float_empty pass_agg_float_empty(struct agg_float_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+struct agg_nofloat_emptyarray { float a; [[no_unique_address]] empty dummy[3]; };
+struct agg_nofloat_emptyarray pass_agg_nofloat_emptyarray(struct agg_nofloat_emptyarray arg) { return arg; }
+// CHECK-LABEL: define void @_Z27pass_agg_nofloat_emptyarray22agg_nofloat_emptyarray(%struct.agg_nofloat_emptyarray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z27pass_agg_nofloat_emptyarray22agg_nofloat_emptyarray(%struct.agg_nofloat_emptyarray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+
+// And likewise for members of base classes.
+struct noemptybase { empty dummy; };
+struct agg_nofloat_emptybase : noemptybase { float a; };
+struct agg_nofloat_emptybase pass_agg_nofloat_emptybase(struct agg_nofloat_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct emptybase { [[no_unique_address]] empty dummy; };
+struct agg_float_emptybase : emptybase { float a; };
+struct agg_float_emptybase pass_agg_float_emptybase(struct agg_float_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+struct noemptybasearray { [[no_unique_address]] empty dummy[3]; };
+struct agg_nofloat_emptybasearray : noemptybasearray { float a; };
+struct agg_nofloat_emptybasearray pass_agg_nofloat_emptybasearray(struct agg_nofloat_emptybasearray arg) { return arg; }
+// CHECK-LABEL: define void @_Z31pass_agg_nofloat_emptybasearray26agg_nofloat_emptybasearray(%struct.agg_nofloat_emptybasearray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z31pass_agg_nofloat_emptybasearray26agg_nofloat_emptybasearray(%struct.agg_nofloat_emptybasearray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -486,12 +486,13 @@
   return Ctx.getOrInsertSyncScopeID(""); /* default sync scope */
 }
 
-static bool isEmptyRecord(ASTContext , QualType T, bool AllowArrays);
+static bool isEmptyRecord(ASTContext , QualType T, bool AllowArrays,
+  bool AllowNoUniqueAddr = false);
 
 /// isEmptyField - Return true iff a the field is "empty", that is it
 /// is an unnamed bit-field or an (array of) empty record(s).
 static bool isEmptyField(ASTContext , const FieldDecl *FD,
- bool AllowArrays) {
+ bool AllowArrays, bool AllowNoUniqueAddr = false) {
   if (FD->isUnnamedBitfield())
 return true;
 
@@ -499,11 +500,15 @@
 
   // Constant arrays of empty 

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand marked 2 inline comments as done.
uweigand added a comment.

In D81583#2137277 , @efriedma wrote:

> I'm tempted to say this is a bugfix for the implementation of 
> no_unique_address, and just fix it globally for all ABIs.  We're never going 
> to get anything done here if we require a separate patch for each ABI variant 
> clang supports.


Well, I can certainly do that.  Would you prefer me to completely remove the 
AllowNoUniqueAddress parameter, or keep it but default to true (so ABIs can 
still override if necessary)?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D81583#2138127 , @qiucf wrote:

> Thanks for this patch!
>
> If I understand correctly, only `isEmptyRecord`/`isEmptyField` and places 
> checking any field is zero bit-width may need change for this? Since 
> `isSingleElementStruct` and `isHomogeneousAggregate` just use them to skip 
> empty fields in aggregate. I didn't see direct checking for empty fields on 
> PowerPC. So all change needed on PPC seems to be generic. By enabling 
> `AllowNoUniqueAddr` in these methods, case in 
> https://lists.llvm.org/pipermail/llvm-dev/2020-July/143141.html can be 
> correctly recognized as homogeneous aggregate.


I agree that probably the only required change is to set the 
`AllowNoUniqueAddr` parameter to true in those methods.  Given the discussion 
with @efriedma above, that may actually happen automatically if we remove the 
parameter (or default it to true).

In any case, I guess it would still be good to also have test cases for this 
aspect of the ABI on Power ...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Another ping ...

See also http://lists.llvm.org/pipermail/cfe-dev/2020-July/066162.html


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand closed this revision.
uweigand added a comment.

Committed as 4c5a93bd58bad70e91ac525b0e020bd5119a321a 
.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D80833#2123508 , @aganea wrote:

> In D80833#2109172 , @uweigand wrote:
>
> > Hmm, with clang-cl it seems the driver is trying to use this:
> >  Target: s390x-pc-windows-msvc
> >  which of course doesn't exist.  Not sure what is supposed to be happening 
> > here, but it seems that it's falling back on s390x-linux since on s390x, 
> > Linux is currently the only supported OS.
>
>
> I'm seeing some of the tests are setting the target explicitly `%clang_cl 
> --target=x86_64-windows-msvc`. Would that work on your machine? Or should I 
> do `UNSUPPORTED: s390x` ?


Sorry, looks like I missed this.  I think using an explicit target should work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80833/new/

https://reviews.llvm.org/D80833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand updated this revision to Diff 276151.
uweigand added a comment.

Rebased against mainline.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/systemz-abi.cpp

Index: clang/test/CodeGen/systemz-abi.cpp
===
--- clang/test/CodeGen/systemz-abi.cpp
+++ clang/test/CodeGen/systemz-abi.cpp
@@ -23,3 +23,28 @@
 // CHECK-LABEL: define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, float %{{.*}})
 // SOFT-FLOAT-LABEL:  define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, i32 %{{.*}})
 
+
+// A field member of empty class type in C++ makes the record nonhomogeneous,
+// unless it is marked as [[no_unique_address]];
+struct empty { };
+struct agg_nofloat_empty { float a; empty dummy; };
+struct agg_nofloat_empty pass_agg_nofloat_empty(struct agg_nofloat_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct agg_float_empty { float a; [[no_unique_address]] empty dummy; };
+struct agg_float_empty pass_agg_float_empty(struct agg_float_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+
+// And likewise for members of base classes.
+struct noemptybase { empty dummy; };
+struct agg_nofloat_emptybase : noemptybase { float a; };
+struct agg_nofloat_emptybase pass_agg_nofloat_emptybase(struct agg_nofloat_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct emptybase { [[no_unique_address]] empty dummy; };
+struct agg_float_emptybase : emptybase { float a; };
+struct agg_float_emptybase pass_agg_float_emptybase(struct agg_float_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -486,12 +486,13 @@
   return Ctx.getOrInsertSyncScopeID(""); /* default sync scope */
 }
 
-static bool isEmptyRecord(ASTContext , QualType T, bool AllowArrays);
+static bool isEmptyRecord(ASTContext , QualType T, bool AllowArrays,
+  bool AllowNoUniqueAddr = false);
 
 /// isEmptyField - Return true iff a the field is "empty", that is it
 /// is an unnamed bit-field or an (array of) empty record(s).
 static bool isEmptyField(ASTContext , const FieldDecl *FD,
- bool AllowArrays) {
+ bool AllowArrays, bool AllowNoUniqueAddr = false) {
   if (FD->isUnnamedBitfield())
 return true;
 
@@ -514,16 +515,23 @@
   //
   // FIXME: We should use a predicate for whether this behavior is true in the
   // current ABI.
-  if (isa(RT->getDecl()))
+  //
+  // The exception to the above rule are fields marked with the
+  // [[no_unique_address]] attribute (since C++20).  Those do count
+  // as empty according to the Itanium ABI.  This property is currently
+  // only respected if the AllowNoUniqueAddr parameter is true.
+  if (isa(RT->getDecl()) &&
+  !(AllowNoUniqueAddr && FD->hasAttr()))
 return false;
 
-  return isEmptyRecord(Context, FT, AllowArrays);
+  return isEmptyRecord(Context, FT, AllowArrays, AllowNoUniqueAddr);
 }
 
 /// isEmptyRecord - Return true iff a structure contains only empty
 /// fields. Note that a structure with a flexible array member is not
 /// considered empty.
-static bool isEmptyRecord(ASTContext , QualType T, bool AllowArrays) {
+static bool isEmptyRecord(ASTContext , QualType T, bool AllowArrays,
+  bool AllowNoUniqueAddr) {
   const RecordType *RT = T->getAs();
   if (!RT)
 return false;
@@ -534,11 +542,11 @@
   // If this is a C++ record, check the bases first.
   if (const CXXRecordDecl *CXXRD = dyn_cast(RD))
 for (const auto  : 

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

> Line 4 here fails on s390x but not on other Unix flavors, see: 
> http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33346/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adebug-info-codeview-buildinfo.c
> 
> @thakis @uweigand Any ideas would could go wrong here?

Well, when I try it, the compile command is creating a **native** format object 
file, in this case a 64-bit big-endian s390x ELF file, and that format is not 
recognized by llvm-pdbutil.  Not sure why this wouldn't fail the same way on 
any other non-Windows target, though.

I guess it should work if you pass the appropriate --target option to the 
compiler to force creation of a Windows object file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80833/new/

https://reviews.llvm.org/D80833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Ping?   I'd really appreciate feedback about this ABI issue, in particular from 
other affected target maintainers ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81583/new/

https://reviews.llvm.org/D81583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Hmm, with clang-cl it seems the driver is trying to use this:
Target: s390x-pc-windows-msvc
which of course doesn't exist.  Not sure what is supposed to be happening here, 
but it seems that it's falling back on s390x-linux since on s390x, Linux is 
currently the only supported OS.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80833/new/

https://reviews.llvm.org/D80833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78644: [LSan] Enable for SystemZ

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

This all looks good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78644/new/

https://reviews.llvm.org/D78644



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand created this revision.
uweigand added reviewers: craig.topper, erichkeane, jasonliu, kbarton, rnk, 
asl, sunfish, t.p.northover, arsenm, asb.
Herald added subscribers: cfe-commits, wdng.
Herald added a project: clang.

The SystemZ ABI has special cases to handle structs containing just a single 
floating-point member.  In determining this property, there are corner cases 
around empty fields.  So for example, if a struct contains an "empty member" in 
addition to a member of floating-point type, the struct is still considered to 
contain just a single floating-point member.

In (prior versions of) C++, however, members of class type would never count as 
"empty" given the C++ rules that even an empty class should be considered as 
having a size of at least 1.   But now with C++20, data members can be marked 
with the [[no_unique_address]] attribute, in which case that rule no longer 
applies.  The Itanium ABI document was updated to address this new situation 
(https://itanium-cxx-abi.github.io/cxx-abi/abi.html#definitions):

> 
> 
>   empty class
>  A class with no non-static data members other than empty data members, 
> no unnamed bit-fields other than zero-width bit-fields, no virtual functions, 
> no virtual base classes, and no non-empty non-virtual proper base classes.
>
> 
> empty data member
> 
>   A potentially-overlapping non-static data member of empty class type. 

GCC 10 has been updated across platforms to respect this new case.

This patch implements the new case in the ABI code for SystemZ.  Note that I'm 
changing common subroutines (isEmptyField / isEmptyRecord) that are used for 
other ABIs as well.  To prevent this patch from having any unintended effect on 
other platforms, I've guarded the new behavior with an extra flag that is 
currently only set on SystemZ.

Now I would expect that most other platforms who use isEmptyField / 
isEmptyRecord / isSingleElementStruct / isHomogeneousAggregate will also have 
to respect that new behavior in order to stay compatible with the respective 
system ABIs (and GCC), but I'd prefer to leave this up to the maintainers of 
those platforms ...


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81583

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/systemz-abi.cpp

Index: clang/test/CodeGen/systemz-abi.cpp
===
--- clang/test/CodeGen/systemz-abi.cpp
+++ clang/test/CodeGen/systemz-abi.cpp
@@ -9,3 +9,28 @@
 struct agg_float_cpp pass_agg_float_cpp(struct agg_float_cpp arg) { return arg; }
 // CHECK-LABEL: define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, float %{{.*}})
 // SOFT-FLOAT:  define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+
+// A field member of empty class type in C++ makes the record nonhomogeneous,
+// unless it is marked as [[no_unique_address]];
+struct empty { };
+struct agg_nofloat_empty { float a; empty dummy; };
+struct agg_nofloat_empty pass_agg_nofloat_empty(struct agg_nofloat_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT:  define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct agg_float_empty { float a; [[no_unique_address]] empty dummy; };
+struct agg_float_empty pass_agg_float_empty(struct agg_float_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT:  define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+
+// And likewise for members of base classes.
+struct noemptybase { empty dummy; };
+struct agg_nofloat_emptybase : noemptybase { float a; };
+struct agg_nofloat_emptybase pass_agg_nofloat_emptybase(struct agg_nofloat_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT:  define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct emptybase { [[no_unique_address]] empty dummy; };
+struct agg_float_emptybase : emptybase { float a; };
+struct agg_float_emptybase pass_agg_float_emptybase(struct agg_float_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT:  define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+
Index: 

[PATCH] D84341: Implement __builtin_eh_return_data_regno for SystemZ

[Ulrich Weigand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7f003957bfcd: [SystemZ] Implement 
__builtin_eh_return_data_regno (authored by uweigand).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84341/new/

https://reviews.llvm.org/D84341

Files:
  clang/lib/Basic/Targets/SystemZ.h
  clang/test/CodeGen/builtins-systemz.c


Index: clang/test/CodeGen/builtins-systemz.c
===
--- clang/test/CodeGen/builtins-systemz.c
+++ clang/test/CodeGen/builtins-systemz.c
@@ -142,3 +142,10 @@
   result = __TM_failure_code (tdb);
 }
 
+void test_eh_return_data_regno() {
+  volatile int res;
+  res = __builtin_eh_return_data_regno(0); // CHECK: store volatile i32 6
+  res = __builtin_eh_return_data_regno(1); // CHECK: store volatile i32 7
+  res = __builtin_eh_return_data_regno(2); // CHECK: store volatile i32 8
+  res = __builtin_eh_return_data_regno(3); // CHECK: store volatile i32 9
+}
Index: clang/lib/Basic/Targets/SystemZ.h
===
--- clang/lib/Basic/Targets/SystemZ.h
+++ clang/lib/Basic/Targets/SystemZ.h
@@ -157,6 +157,10 @@
   const char *getLongDoubleMangling() const override { return "g"; }
 
   bool hasExtIntType() const override { return true; }
+
+  int getEHDataRegisterNumber(unsigned RegNo) const override {
+return RegNo < 4 ? 6 + RegNo : -1;
+  }
 };
 } // namespace targets
 } // namespace clang


Index: clang/test/CodeGen/builtins-systemz.c
===
--- clang/test/CodeGen/builtins-systemz.c
+++ clang/test/CodeGen/builtins-systemz.c
@@ -142,3 +142,10 @@
   result = __TM_failure_code (tdb);
 }
 
+void test_eh_return_data_regno() {
+  volatile int res;
+  res = __builtin_eh_return_data_regno(0); // CHECK: store volatile i32 6
+  res = __builtin_eh_return_data_regno(1); // CHECK: store volatile i32 7
+  res = __builtin_eh_return_data_regno(2); // CHECK: store volatile i32 8
+  res = __builtin_eh_return_data_regno(3); // CHECK: store volatile i32 9
+}
Index: clang/lib/Basic/Targets/SystemZ.h
===
--- clang/lib/Basic/Targets/SystemZ.h
+++ clang/lib/Basic/Targets/SystemZ.h
@@ -157,6 +157,10 @@
   const char *getLongDoubleMangling() const override { return "g"; }
 
   bool hasExtIntType() const override { return true; }
+
+  int getEHDataRegisterNumber(unsigned RegNo) const override {
+return RegNo < 4 ? 6 + RegNo : -1;
+  }
 };
 } // namespace targets
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84341: Implement __builtin_eh_return_data_regno for SystemZ

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM with the format fixes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84341/new/

https://reviews.llvm.org/D84341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82862: [ThinLTO] Always parse module level inline asm with At dialect

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.
Herald added a subscriber: pengfei.

Hi @hans , we're having some issues with using the AssemblerDialect mechanism 
to support both the GNU assembler and the IBM HLASM assembler in the SystemZ 
back-end. See also the discussion started here: 
https://lists.llvm.org/pipermail/llvm-dev/2020-November/146875.html

One of the issues that showed up is what you're refering to above:

> That flag however should not affect the *parsing* of inline assembly in the 
> program.

I'm wondering why this is true.  I mean, it is true that the flag currently 
does not affect parsing of inline asm, but I'm wondering whether it *should* be 
that way.

Note that the `-x86-asm-syntax=intel` LLVM flag is used to implement the 
`-masm=intel` clang **command line option**, which exists also in GCC and where 
hopefully the two compilers should be compatible.  But in GCC, that flag is 
documented to affect parsing of inline assembly just like it affects output.   
See https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/x86-Options.html#x86-Options

> -masm=dialect
>
>   Output assembly instructions using selected dialect. Also affects which 
> dialect is used for basic asm (see Basic Asm) and extended asm (see Extended 
> Asm). Supported choices (in dialect order) are ‘att’ or ‘intel’. The default 
> is ‘att’. Darwin does not support ‘intel’.

What is the reason for treating this differently in LLVM?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82862/new/

https://reviews.llvm.org/D82862

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82862: [ThinLTO] Always parse module level inline asm with At dialect

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D82862#2500717 , @hans wrote:

>> In D82862#2498785 , @hans wrote:
>>
>>> The motivation for my change was really just to make ThinLTO compiles work 
>>> the same as non-ThinLTO ones.
>>>
>>> Maybe the fact that -x86-asm-syntax=intel doesn't affect inline asm is a 
>>> bug. I wasn't aware that Clang and GCC's -masm= flags behaved differently 
>>> in that way, but that certainly suggests there's a problem here.
>>
>> So I'm wondering, if I remove the above setAssemblerDialect line **and** 
>> revert this commit, we should have inline asm (both module-level and GNU 
>> function-leve) respect the target-selected asm dialect variant both for 
>> ThinLTO and non-ThinLTO, so they should match again.   Would that also solve 
>> the problem you were originally tracking?
>
> Not completely, because clang-cl sets -x86-asm-syntax=intel to enable 
> intel-style asm in assembly listing output. We'd have to find another way of 
> doing that without affecting the inline asm dialect.

So why do you want GNU inline asm for clang-cl anyway?   I thought the whole 
point of clang-cl was to be compatible with the Microsoft Visual Studio 
compiler, which I understand only supports the MS asm syntax?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82862/new/

https://reviews.llvm.org/D82862

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82862: [ThinLTO] Always parse module level inline asm with At dialect

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D82862#2500038 , @pengfei wrote:

>> What is the reason for treating this differently in LLVM?
>
> I'm not sure if it is related to this. I think one difference is that LLVM is 
> supporting MS inline assembly. Although it uses Intel dialect, it has 
> different representation in input, output, clobber etc. with GCC'.

That is indeed another complication.  On the one hand, we have two different 
inline asm formats, GNU assembly and MS assembly.  These differ completely in 
how asm arguments (passed in from surrounding C/C++ code) are handled and 
therefore need to be parsed quite differently.  On the other hand, we have the 
different assembler dialects (AT vs. Intel on x86), which affect how the 
single asm instructions are to be parsed.

Unfortunately, the distinction between the two seems to be somewhat muddled in 
LLVM at this point.  In particular, MS assembly statements are represented at 
the LLVM IR level via the "inteldialect" token.  This is a bit confusing 
because while MS asm indeed always uses the Intel dialect, we can also have GNU 
asm with the Intel dialect -- at least this is the case in GCC when using 
-masm=intel.

What I think we should have is:

- the LLVM IR level distinction between GNU and MS asm statements; and in 
addition
- for GNU asm statements, a target-specific selection of assembler variants, 
which may depend on the target triple and/or command line options like -masm=

If you look at AsmPrinter::emitInlineAsm, this is actually **partially** 
implemented already:

  // The variant of the current asmprinter.
  int AsmPrinterVariant = MAI->getAssemblerDialect();
  AsmPrinter *AP = const_cast(this);
  if (MI->getInlineAsmDialect() == InlineAsm::AD_ATT)
EmitGCCInlineAsmStr(AsmStr, MI, MMI, AsmPrinterVariant, AP, LocCookie, OS);
  else
EmitMSInlineAsmStr(AsmStr, MI, MMI, AP, LocCookie, OS);

So here the LLVM IR marker is used to select between GCC and MS inline asm 
instructions, and for the GCC inline asm statements, the target is queried as 
to the proper asm dialect variant to use.

However, later on we have a

  Parser->setAssemblerDialect(Dialect);

call, where the Dialect is again taken from the LLVM IR setting -- so for GNU 
asm, this gets now always set back to AT   It seems to me that this is 
simply wrong.

Then, we finally have **module** level inline asm statements, which are always 
treated as GNU style (although this may not really make any difference since 
module level statements do not have arguments anyway).  Again because of the 
above setAssemblerDialect call they would therefore be also treated as AT 
dialect, which I guess is what @hans originally noticed.

In D82862#2498785 , @hans wrote:

> The motivation for my change was really just to make ThinLTO compiles work 
> the same as non-ThinLTO ones.
>
> Maybe the fact that -x86-asm-syntax=intel doesn't affect inline asm is a bug. 
> I wasn't aware that Clang and GCC's -masm= flags behaved differently in that 
> way, but that certainly suggests there's a problem here.

So I'm wondering, if I remove the above setAssemblerDialect line **and** revert 
this commit, we should have inline asm (both module-level and GNU 
function-leve) respect the target-selected asm dialect variant both for ThinLTO 
and non-ThinLTO, so they should match again.   Would that also solve the 
problem you were originally tracking?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82862/new/

https://reviews.llvm.org/D82862

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82862: [ThinLTO] Always parse module level inline asm with At dialect

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D82862#2513044 , @rnk wrote:

> In D82862#2512908 , @uweigand wrote:
>
>> So why do you want GNU inline asm for clang-cl anyway?   I thought the whole 
>> point of clang-cl was to be compatible with the Microsoft Visual Studio 
>> compiler, which I understand only supports the MS asm syntax?
>
> We have users, in this case, I think it's V8, who would prefer to use 
> gcc-style module level assembly if it is available. Their motivation is 
> somewhat arbitrary, but generally, clang-cl supports a variety of extensions, 
> some inherited from GCC, in all modes. Part of the point of switching 
> compilers from MSVC to clang is to get access to those extensions.

I see, thanks.   I think what would make sense to preserve existing behavior 
while still allowing other platforms to use different dialects for GNU inline 
asm would be to move the decision which dialect to use for inline asm to the 
back-end.  I've posted a patch along those lines as 
https://reviews.llvm.org/D95444 - I hope we can continue the discussion there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82862/new/

https://reviews.llvm.org/D82862

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Looks like this is also failing on s390x:

  error: Added modules have incompatible data layouts: 
E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64 (module) vs 
E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64 (jit)

The problem here is that on s390x we use a different data layout on machines 
with vector registers vs. machines without.   The (module) string above is the 
version without vector registers (which is presumably selected because there is 
no -march= argument and the compiler therefore defaults to an old machine), and 
the (jit) string is the version with vector registers (which is presumably 
because the jit auto-detected that it is running on a new machine).

I guess we should either tell the JIT to not autodetect the current processor, 
or else tell the compiler to target the processor that the JIT autodetected?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96033/new/

https://reviews.llvm.org/D96033

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D96033#2765954 , @v.g.vassilev 
wrote:

> @hubert.reinterpretcast, thanks for the feedback. I have created a patch as 
> discussed -- https://reviews.llvm.org/D102688
>
> @uweigand, thanks for reaching out. I believe the patch above should fix your 
> setup. Could you confirm?

Unfortunately, it does not.  Changing the triple doesn't affect the 
architecture the compiler generates code for.   If you wanted to change the 
compiler to generate code for the architecture the JIT detects, the easiest way 
would probably be to use (the equivalent of) "-march=native", which causes the 
compiler to also auto-detect the current processor in the same way as the JIT 
does.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96033/new/

https://reviews.llvm.org/D96033

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Yes, this patch fixes the problem for me.  Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96033/new/

https://reviews.llvm.org/D96033

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102064: Parse vector bool when stdbool.h and altivec.h are included

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D102064#2751089 , @ZarkoCA wrote:

> In D102064#2751001 , @uweigand 
> wrote:
>
>> This means the implementation now deviates from the documented vector 
>> extension syntax, right?   I guess it's strictly an extension of the 
>> documented syntax, but that may still lead to incompatibilities with other 
>> compilers for the platform.  If we want to make such a change, should it be 
>> synchronized with e.g. GCC, XL, etc. ?
>
> GCC and XL already accept this syntax on Linux on Power and AIX.
>
> For example this simple test case:
>
>   #include 
>   #include 
>   
>   vector bool char bc;
>
> Can compile with GCC 9/10 and XLC 16.1 on Linux on Power. On AIX, GCC 8.3 on 
> AIX and XLC 16.1 can also compile it successfully.  Latest main trunk clang 
> throws up an error on those platforms.
>
> From offline conversation it looks like XLC on z/OS can also compile the test 
> case. @Everybody0523 can confirm for sure.

Interesting.   On Z using GCC I currently get this error:

  vbool.c:2:1: error: invalid vector type for attribute ‘vector_size’
  2 | vector _Bool x;

But looking at the GCC sources, it seems we actually intended to support this 
use as well, there's just a bug.   Given that, I think I'd be fine with adding 
this to LLVM -- I'll make sure the GCC bug gets fixed as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102064/new/

https://reviews.llvm.org/D102064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102064: Parse vector bool when stdbool.h and altivec.h are included

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

This means the implementation now deviates from the documented vector extension 
syntax, right?   I guess it's strictly an extension of the documented syntax, 
but that may still lead to incompatibilities with other compilers for the 
platform.  If we want to make such a change, should it be synchronized with 
e.g. GCC, XL, etc. ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102064/new/

https://reviews.llvm.org/D102064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97901: [SystemZ] Test for infinity in testFPKind().

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97901/new/

https://reviews.llvm.org/D97901

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97901: [SystemZ] Test for infinity in testFPKind().

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:7229
+  case Builtin::BI__builtin_isfinite:
+Invert = true;
+LLVM_FALLTHROUGH;

thopre wrote:
> jonpa wrote:
> > What are these variants all about...?
> > 
> They were introduced in https://reviews.llvm.org/D24483
This "invert" logic doesn't look correct.   "isfinite" and "isinf" **both** 
need to return false on NaNs.  I think you should just drop the invert logic 
and use a TDC mask of 0xFC0 (zero, normal, or subnormal) to implement 
"isfinite".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97901/new/

https://reviews.llvm.org/D97901

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96568: [CFE, SystemZ] Emit s390.tdc instrincic for __builtin_isnan in Constrained FP mode.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM as well, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96568/new/

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109362: [SystemZ][z/OS] Add GOFF Support to the DataLayout

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

SystemZ specific parts LGTM, but it would be good to have someone else review 
the common code / IR changes as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109362/new/

https://reviews.llvm.org/D109362

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109362: [SystemZ][z/OS] Add GOFF Support to the DataLayout

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.

This LGTM now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109362/new/

https://reviews.llvm.org/D109362

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109362: [SystemZ][z/OS] Add GOFF Support to the DataLayout

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: llvm/docs/LangRef.rst:2596
 * ``e``: ELF mangling: Private symbols get a ``.L`` prefix.
+* ``l``: GOFF mangling: Private symbols get a ``.L`` prefix.
 * ``m``: Mips mangling: Private symbols get a ``$`` prefix.

Comment needs to be update now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109362/new/

https://reviews.llvm.org/D109362

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109362: [SystemZ][z/OS] Add GOFF Support to the DataLayout

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Looking at the common code parts, it seems the behavior of MM_GOFF is actually 
identical to MM_ELF.   Is this correct?   If so, do we really need a different 
format type here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109362/new/

https://reviews.llvm.org/D109362

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109362: [SystemZ][z/OS] Add GOFF Support to the DataLayout

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D109362#3000284 , @anirudhp wrote:

> In D109362#2999688 , @uweigand 
> wrote:
>
>> Looking at the common code parts, it seems the behavior of MM_GOFF is 
>> actually identical to MM_ELF.   Is this correct?   If so, do we really need 
>> a different format type here?
>
> At a future point, we will be changing the local and global prefixes. At that 
> point we would still need a separate `MM_GOFF` field. I feel it would be a 
> bit better to enforce it from now itself.

I see.  Is there a reason why we cannot use the "correct" prefixes to begin 
with?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109362/new/

https://reviews.llvm.org/D109362

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105629: [TSan] Add SystemZ support

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

See inline comments ... otherwise the SystemZ platform-specific parts look good 
to me.




Comment at: compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp:123
+#if defined(__s390x__)
+  ProtectRange(HiAppMemEnd(), 0xf000ull);
+#endif

Did you test this on older kernels without 5-level page table support?   I 
believe the allocation / mprotect may fail on those ...



Comment at: compiler-rt/lib/tsan/rtl/tsan_rtl_s390x.S:22
+  CFI_REL_OFFSET(%r2, R2_REL_OFFSET)
+  CFI_REL_OFFSET(%r3, R3_REL_OFFSET)
+  stmg %r14, %r15, R14_REL_OFFSET(%r15)

Do we need CFI for r2/r3 ?  Those are call-clobbered any cannot be unwound 
normally anyway ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105629/new/

https://reviews.llvm.org/D105629

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105629: [TSan] Add SystemZ support

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp:123
+#if defined(__s390x__)
+  ProtectRange(HiAppMemEnd(), 0xf000ull);
+#endif

iii wrote:
> uweigand wrote:
> > Did you test this on older kernels without 5-level page table support?   I 
> > believe the allocation / mprotect may fail on those ...
> No, not really. Would it make sense to probe here? E.g. first try 
> 0xf000, then 0x20. Or is there a way to query 
> user_addr_max() / TASK_SIZE_MAX / TASK_SIZE?
I don't know of any way to query this.  You can simply do the allocation in 
stages (first to the top of the three-pagetable range, then four, then five), 
and ignore failures.   As an unfortunate side effect this will force the kernel 
to allocate five levels of page tables even when unnecessary, but I don't think 
there's anything we can do about that.



Comment at: compiler-rt/lib/tsan/rtl/tsan_rtl_s390x.S:22
+  CFI_REL_OFFSET(%r2, R2_REL_OFFSET)
+  CFI_REL_OFFSET(%r3, R3_REL_OFFSET)
+  stmg %r14, %r15, R14_REL_OFFSET(%r15)

iii wrote:
> uweigand wrote:
> > Do we need CFI for r2/r3 ?  Those are call-clobbered any cannot be unwound 
> > normally anyway ...
> I'm not quite sure, but glibc does this (e.g. in 
> sysdeps/s390/s390-64/dl-trampoline.h), so I figured I'll do this here as well 
> just in case.
Huh.   Well I guess it doesn't hurt either way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105629/new/

https://reviews.llvm.org/D105629

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105629: [TSan] Add SystemZ support

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp:123
+#if defined(__s390x__)
+  ProtectRange(HiAppMemEnd(), 0xf000ull);
+#endif

iii wrote:
> uweigand wrote:
> > iii wrote:
> > > uweigand wrote:
> > > > Did you test this on older kernels without 5-level page table support?  
> > > >  I believe the allocation / mprotect may fail on those ...
> > > No, not really. Would it make sense to probe here? E.g. first try 
> > > 0xf000, then 0x20. Or is there a way to query 
> > > user_addr_max() / TASK_SIZE_MAX / TASK_SIZE?
> > I don't know of any way to query this.  You can simply do the allocation in 
> > stages (first to the top of the three-pagetable range, then four, then 
> > five), and ignore failures.   As an unfortunate side effect this will force 
> > the kernel to allocate five levels of page tables even when unnecessary, 
> > but I don't think there's anything we can do about that.
> 3-level page table limit is quite below the application range defined in this 
> series (0x400 < 0xc000). Are there any relevant kernels that 
> do not support 4-level page tables? I tried to search the git history, and it 
> looks as if even 2.6 ones have it.
Ah, right.  Yes, we can certainly assume 4-level support at this point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105629/new/

https://reviews.llvm.org/D105629

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109362: [SystemZ][z/OS] Add GOFF Support to the DataLayout

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D109362#3013253 , @anirudhp wrote:

> - Reduced the number of test lines in target-data.c, since we don't have to 
> check for every combination of arch,cpu for the SystemZ target (for both elf 
> and z/OS)

These changes still LGTM from a SystemZ target perspective.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109362/new/

https://reviews.llvm.org/D109362

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91944: OpenMP 5.0 metadirective

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

Looks like this was committed again, breaking the SystemZ build bots once again:
https://lab.llvm.org/buildbot/#/builders/94/builds/5661


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91944/new/

https://reviews.llvm.org/D91944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91944: OpenMP 5.0 metadirective

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D91944#3009868 , @cchen wrote:

> The SystemZ issue is due to the fact that we assumed that `device(cpu)` 
> should be evaluated to true and `device(gpu)` should be evaluated to false in 
> the test so the test should be fixed by specifying the triple. 
> (https://github.com/llvm/llvm-project/commit/3679d2001c87f37101e7f20c646b21e97d8a0867)

Thanks, it looks like this fixed the problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91944/new/

https://reviews.llvm.org/D91944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105629: [TSan] Add SystemZ support

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

The SystemZ specific changes all look good to me now, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105629/new/

https://reviews.llvm.org/D105629

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

It looks like this caused the LNT build bot to fail (at least on s390x), 
because one of the test cases now triggers this error:
https://lab.llvm.org/buildbot/#/builders/45/builds/6787

  /usr/bin/make -f 
SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeFiles/GCC-C-execute-ieee-compare-fp-4.dir/build.make
 
SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeFiles/GCC-C-execute-ieee-compare-fp-4.dir/build
  
/home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/test-suite/SingleSource/Regression/C/gcc-c-torture/execute/ieee/20030331-1.c:6:1:
 error: conflicting types for 'rintf'
  rintf ()
  ^
  
/home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/test-suite/SingleSource/Regression/C/gcc-c-torture/execute/ieee/20030331-1.c:6:1:
 note: previous implicit declaration is here
  make[2]: Entering directory 
'/home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build'
  
/home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/test-suite/SingleSource/Regression/C/gcc-c-torture/execute/ieee/20030331-1.c:29:14:
 error: too few arguments to function call, expected 1, have 0
if (rintf () != -2.0)
~  ^
  2 errors generated.

I guess the error is valid, but should either be disabled for this LNT test, or 
else the test fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123627/new/

https://reviews.llvm.org/D123627

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D123627#3451373 , @aaron.ballman 
wrote:

> Thank you for letting me know -- I've speculatively fixed the issue in 
> 726901d06aab2f92d684d28507711308368c29d6 
> 

Yes, the build bot is now green again.  Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123627/new/

https://reviews.llvm.org/D123627

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158883: [Matrix] Try to emit fmuladd for both vector and matrix types

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D158883#4638648 , @thegameg wrote:

> In D158883#4635997 , @uweigand 
> wrote:
>
>> The newly added test cases in ffp-model.c fail on SystemZ, making CI red:
>
> Should be fixed, thanks for the report and sorry for the delay.

Thanks, this fixes the problem for me locally.  (Build bot is unfortunately 
still down because of this: 
https://github.com/llvm/llvm-project/pull/65267#issuecomment-1707318337)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158883/new/

https://reviews.llvm.org/D158883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158883: [Matrix] Try to emit fmuladd for both vector and matrix types

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

The newly added test cases in ffp-model.c fail on SystemZ, making CI red:
https://lab.llvm.org/buildbot/#/builders/94/builds/16280

The root cause seems to be that by default, the SystemZ back-end targets a 
machine without SIMD support, and therefore vector return types are passed via 
implicit reference according to the ABI:

  
/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/clang/test/CodeGen/ffp-model.c:121:12:
 error: CHECK: expected string not found in input
   // CHECK: define{{.*}} <4 x float> @my_m22_muladd
 ^
  :62:28: note: scanning from here
   %4 = fadd fast <2 x float> %2, %3
 ^
  :67:1: note: possible intended match here
  define dso_local void @my_m22_muladd(ptr noalias sret([4 x float]) align 4 
%agg.result, ptr noundef %0, float noundef nofpclass(nan inf) %y, ptr noundef 
%1) #0 {
  ^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158883/new/

https://reviews.llvm.org/D158883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127498: [SystemZ/z/OS] Set DWARF version to 4 for z/OS.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127498/new/

https://reviews.llvm.org/D127498

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits