[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] D149548: [IR] Update to use new shufflevector semantics

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D149548#4413639 , @nlopes wrote:

> If a vector is fully initialized with `insertvector` (i.e., one operation per 
> index), then the value of the base vector is irrelevant. It can be poison.
> Poison in vectors is element-wise.  doesn't propagate to  poison>.

OK, that should be fine for SystemZ then.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149548

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

[PATCH] D149548: [IR] Update to use new shufflevector semantics

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

So the semantics of the `vec_promote(a, b)` intrinsic is defined as:

> Returns a vector with a in element position b. The result is a vector with a 
> in element position b. [...] The other elements of the vector are undefined.

This is currently implemented by using `insertvector` to place `a` at position 
`b` into a source vector that is `undef`.   The effect should be that when 
using element `b` of that vector, we are guaranteed to get `a`, while using any 
other element is undefined behavior (just like accessing an uninitialized 
variable).

To be honest, I'm not sure how exactly the LLVM IR semantics changes here when 
using a `poison` source vector instead of `undef`.  I seem to recall that 
`poison` propagates over operations - is it true that the result of 
`insertvector` on a `poison` vector is itself `poison`?  If so, then this 
change would break semantics.   However, if the result is a vector with `a` in 
element `b`, and `poison` only in the other elements, then I guess this would 
still preserve the expected semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149548

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

[PATCH] D139444: [ZOS] Convert tests to check 'target={{.*}}-zos'

[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/D139444/new/

https://reviews.llvm.org/D139444

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

[PATCH] D139444: [ZOS] Convert tests to check 'target={{.*}}-zos'

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D139444#3983679 , @uweigand wrote:

> In D139444#3982205 , @probinson 
> wrote:
>
>> If you can tell me the `platform.system()` value to look for to detect z/OS, 
>> I can do that.
>
> I don't know - this value is defined by the Python implementation on z/OS.  
> On most platforms, this matches the value returned by "uname -s", but I don't 
> know what this is on z/OS either.

@Kai - maybe you can help with this.


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

https://reviews.llvm.org/D139444

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

[PATCH] D139444: [ZOS] Convert tests to check 'target={{.*}}-zos'

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D139444#3982205 , @probinson wrote:

> If you can tell me the `platform.system()` value to look for to detect z/OS, 
> I can do that.

I don't know - this value is defined by the Python implementation on z/OS.  On 
most platforms, this matches the value returned by "uname -s", but I don't know 
what this is on z/OS either.


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

https://reviews.llvm.org/D139444

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

[PATCH] D139444: [ZOS] Convert tests to check 'target={{.*}}-zos'

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D139444#3975182 , @probinson wrote:

> The changes in this patch assume that there aren't any possible suffixes 
> after the `-zos` part of the triple (no version numbers, like you might find 
> with darwin or macos, and nothing like `-elf` or `-eabi` like some targets 
> have).  If there are suffixes, I'll happily revise to put `{{.*}}` after 
> everything.

I think for consistency with other targets, and to be safe for future 
extensions of the target triple, it would be better to add the `{{.*}}`

> The one test I could not verify is llvm/test/Support/encoding.ll, because I 
> don't have the utilities that it needs.  But `UNSUPPORTED: !` was the 
> workaround for not having triples allowed in `REQUIRES:` so I think it's the 
> right change.

The question here is, what are the specific requirements for the test to run.   
One part is that the test just calls plain `llc` but expects this will generate 
code appropriate for z/OS.  That's not how this is usually done.  Instead, you 
should add an explicit target triple to the `llc` invocation, e.g. `llc 
-mtriple=s390x-ibm-zos`.   However, as this requires that support for the 
SystemZ target is built into LLVM, you then also need to add the following:

  REQUIRES: systemz-registered-target

This has the advantage that the test will actually be run on any host platform, 
as long as the target support is built in (which is is by default e.g. in the 
CI builders).

However, it might also be the case that the test case requires a z/OS host 
environment to run on.  I believe this is true here, since `chtag` seems to be 
a z/OS specific tool, and `iconv` (in particular when using the `IBM-1047` code 
page) also may not be universally available.

To express that restriction on the *host* system, you should be using a 
`REQUIRES: system-zos` line.   However, it looks like this capability is not 
actually currently implemented - you'll have to add it to the code in 
`utils/lit/lit/llvm/config.py` here:

  [...]
  elif platform.system() == 'NetBSD':
  features.add('system-netbsd')
  elif platform.system() == 'AIX':
  features.add('system-aix')
  elif platform.system() == 'SunOS':
  features.add('system-solaris')

(Note that you probably still should add the `-mtriple` because the test case 
requires *both* running on a z/OS host *and* compiling for the z/OS target.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139444

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

[PATCH] D136145: [IR][RFC] Restrict read only when cache type of llvm.prefetch is instruction

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: llvm/test/CodeGen/SystemZ/prefetch-01.ll:18
 
-; Check that instruction write prefetches are ignored.
-define dso_local void @f2(ptr %ptr) {
-; CHECK-LABEL: f2:
-; CHECK-NOT: %r2
-; CHECK: br %r14
-  call void @llvm.prefetch(ptr %ptr, i32 1, i32 0, i32 0)
-  ret void
-}
+; Instruction write prefetches are invalid.
+; define dso_local void @f2(ptr %ptr) {

If we decide to declare this invalid, then I'd prefer to remove the test 
instead of commenting it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136145

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

[PATCH] D136040: [X86] Support PREFETCHI instructions

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D136040#3862386 , @pengfei wrote:

> Sure, it is possible. But at least for now, there's no real target requires 
> it. Checked with `grep -rwn 'llvm.prefetch.*i32 0\s*)' llvm/test/CodeGen/`.

But that's just within the LLVM sources.  My point was that -up to now- this 
was part of the public LLVM IR spec, so there could by other LLVM users out 
there creating this IR, which would now suddenly break.  Not sure what the 
rules are for breaking IR changes, but that would presumably need some wider 
discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136040

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

[PATCH] D136040: [X86] Support PREFETCHI instructions

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D136040#3862225 , @pengfei wrote:

> 3. Add semacheck for prefetch write to instruction cache;
>
> I think the affected ARM and SystemZ tests are not valid before. Could 
> @t.p.northover and @uweigand help to have a look?

This seems to be a semantic change.   The Language Reference does not spell out 
that a write prefetch on the instruction cache is prohibited.  In fact, I read 
it to explicitly state that every flavor of the prefetch intrinsic that doesn't 
match anything supported on the target architecture should simply be a no-op.  
(I could imagine that on certain architectures, you might even have such a 
prefetch, e.g. to handle self-modifying code more efficiently.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136040

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

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

This LGTM now, but maybe some of the PowerPC reviewers would like to comment as 
well.  @nemanjai ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D18

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

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5471
+if (CoerceTy->isIntegerTy() && CoerceTy->getIntegerBitWidth() < 
GPRBits)
+  ForceRightAdjust = true;
+  }

Are all these checks really necessary here?  This seems to duplicate the checks 
that are already in `emitVoidPtrVAArg` ...Can't we simply always pass 
`true` for `ForceRightAdjust` on PowerPC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D18

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

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

I think it is correct to implement this in Clang.   Note that on SystemZ 
(another big-endian platform), we also implement this in `EmitVAArg`.   Of 
course the details are different since we're not using `emitVoidPtrVAArg` on 
that platform.

However, I'm not sure if the implementation itself is quite correct, in 
particular if it is right to just replace the type.  Note that looking into 
`emitVoidPtrVAArg` I see this:

  // If the argument is smaller than a slot, and this is a big-endian
  // target, the argument will be right-adjusted in its slot.
  if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian() &&
  !DirectTy->isStructTy()) {
Addr = CGF.Builder.CreateConstInBoundsByteGEP(Addr, SlotSize - DirectSize);
  }

which seems to implement exactly the same logic, but *without* modifying the 
type as seen in the IR.It looks like the only change needed for ppc would 
be to remove the `!DirectTy->isStructTy()` check here?   (I guess to avoid 
inadvertently change other targets, this might need to be triggered by a flag 
passed as argument.  On the other hand, maybe there is no other big-endian 
platform using `emitVoidPtrVAArg` anyway?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D18

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

[PATCH] D129562: [SystemZ] Enable `-mtune=` option in clang.

[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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129562

___
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

[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] 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] 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.

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] 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] 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.

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] 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.

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] 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] 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] 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 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] 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] 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.

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] 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] 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] 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.
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] 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] 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] 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 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] 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 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 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 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 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] 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] 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] 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.

> 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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 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] 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 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 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] 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] 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] 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] 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] 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] 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] 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 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] 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 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 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 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:
> 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] 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