[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D152914#4653669 , @lei wrote:

> HI @nemanjai, Did you get a chance to post this as a github PR?

Long overdue, but I finally have it up at 
https://github.com/llvm/llvm-project/pull/68919


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D116016: [Clang] [PowerPC] Emit module flag for current float abi

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM. Thanks for reviving this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116016

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D152914#4602914 , @ilinpv wrote:

> Friendly ping, are there any questions remained to proceed with 
> target-independent __builtin_cpu_supports ?

Sorry, I have not gotten back to this review in quite some time as I have been 
relocating to a different country. I'll try to post an updated version of this 
on GitHub this weekend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D158487: [PowerPC][altivec] Optimize codegen of vec_promote

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM. This is a good idea and we should go ahead with this for anyone that uses 
`vec_promote`, but it might be a good idea to improve codegen for the insert 
which might be more common.




Comment at: llvm/test/CodeGen/PowerPC/vec-promote.ll:43
+
+define noundef <4 x float> @vec_promote_float_zeroed(ptr nocapture noundef 
readonly %p) {
+; CHECK-BE-LABEL: vec_promote_float_zeroed:

This code is absolutely terrible. Not only is the `lfs` super slow compared to 
`lfiwzx/lxsiwzx` that we actually want, but the two conversions and three 
permutes are super slow.

I think the change to `altivec.h` to produce better code for something like 
that is a good thing, but I wonder if something like this might come up in 
other contexts.

At least on Power9 and up, we can do much better than this. We don't do 
particularly well regardless of whether we're using a zero vector input or an 
arbitrary vector: https://godbolt.org/z/79fx8nsdP


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158487

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

[PATCH] D158065: [PowerPC] Implement builtin for mffsl

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai accepted this revision.
nemanjai added a comment.

Thanks for implementing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158065

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

[PATCH] D158066: [PowerPC] Fix use of FPSCR builtins in smmintrin.h

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D158066#4593961 , @qiucf wrote:

> CC @amyk @quinnp Any comments about the naming?
>
> I see some `__builtin_ppc_xxx` are aliased into `__builtin_xxx` by 
> `defineXLCompatMacros`. But these are not XL-compatible builtins, and the 
> macros do not always work.

It should be perfectly fine to provide pre-defined macros for these to match 
GCC on PowerPC. The reason we went with the macro solution is to avoid 
polluting the builtins namespace for other targets.

Also, please add some C++ tests for these PPC wrappers so that we aren't 
surprised again when someone tries to use these in their C++ code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158066

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

[PATCH] D156351: clang driver throws error for -mabi=elfv2 or elfv2

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Ah, ok. This makes sense. When we don't know anything about the ABI, it makes 
sense to rely on `-mabi`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156351

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

[PATCH] D156076: [PowerPC][Clang] Remove constraint for initial-exec TLS mode on AIX

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

So we don't have code generation for this, but we are enabling it in the front 
end? What happens if we try to produce code for the IR this produces? It would 
make more sense to me to implement what is needed in the back end prior to 
allowing the front end to produce the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156076

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

[PATCH] D156076: [PowerPC][Clang] Remove constraint for initial-exec TLS mode on AIX

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Please provide a description justifying this change. There is no context here 
for the viewer to determine whether this change makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156076

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

arsenm wrote:
> nemanjai wrote:
> > arsenm wrote:
> > > nemanjai wrote:
> > > > arsenm wrote:
> > > > > From this description I don't understand what this is supposed to do. 
> > > > > What does the input mean? Why does this use an i32 immarg and not a 
> > > > > pointer? Why is the result only i32?
> > > > That is fair enough. The description is fairly vague. I can try to 
> > > > improve it as per below. The parameter for this is not a pointer (i.e. 
> > > > not an address). It is an immediate that represents the index into the 
> > > > enumeration of values that are provided at a fixed address. The back 
> > > > end is then free to produce the actual fixed address and the load 
> > > > itself.
> > > > The choice for the result type was admittedly arbitrary - on PPC, the 
> > > > values provided by GLIBC are 32-bit words.
> > > > 
> > > > Proposed comment describing this intrinsic:
> > > > ```
> > > > // This intrinsic is provided to allow back ends to emit load
> > > > // instructions that load a value from a fixed address. The
> > > > // parameter to the intrinsic is not an address, but an
> > > > // immediate index into an enumeration that contains the
> > > > // union of all such values available on all back ends.
> > > > // An example is the HWCAP/HWCAP2/CPUID words
> > > > // provided by GLIBC on PowerPC to allow fast access
> > > > // to commonly used parts of AUXV. These are provided
> > > > // at a fixed offset into the TCB (accessible through the
> > > > // thread pointer).
> > > > ```
> > > This is baking an a very target specific implementation of device 
> > > identification. Could you redefine this as something more abstract? Like 
> > > returns a device ID integer, or a bool that some int input is supported?
> > The idea is for this to not be restricted to device ID at all, but to be 
> > used for any values that reside in a fixed address for the compiler. For 
> > example, `STACK_GUARD` can be one of the values. How about if the intrinsic 
> > returns any integer type (or maybe any type)?
> > My thinking is that there may be need in the future for various things to 
> > be loaded from fixed addresses. The list of possible things that can be 
> > loaded this way would be a union of what all targets want and only specific 
> > values would make sense on each target.
> But you're assuming there's a fixed address this can be loaded from, and not 
> a read from a special register or some other mechanism
Oh, well sure. My intent was just for those things that are in memory at a 
fixed address. But I suppose you're right, there is probably not a fundamental 
reason to restrict it to things at fixed addresses.

What I am trying to avoid here is defining an intrinsic that takes a specific 
value and returns a `bool` (i.e. the intrinsic version of 
`__builtin_cpu_{supports|is}`. The features/cpus will be different for every 
target and mapping features to integers is also target specific. So this was an 
attempt to implement an intrinsic that gets a value (bit vector if you will) 
that the target can choose how to lower and what to do with the value (i.e. how 
to mask it).

I suppose each target can define their own intrinsics for accessing CPU 
identification information since each target has to provide code generation for 
them anyway.



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6787
   }
+  case Intrinsic::fixed_addr_ld: {
+SDValue Chain = getRoot();

This should check `useLoadFixedAddr()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

arsenm wrote:
> nemanjai wrote:
> > arsenm wrote:
> > > From this description I don't understand what this is supposed to do. 
> > > What does the input mean? Why does this use an i32 immarg and not a 
> > > pointer? Why is the result only i32?
> > That is fair enough. The description is fairly vague. I can try to improve 
> > it as per below. The parameter for this is not a pointer (i.e. not an 
> > address). It is an immediate that represents the index into the enumeration 
> > of values that are provided at a fixed address. The back end is then free 
> > to produce the actual fixed address and the load itself.
> > The choice for the result type was admittedly arbitrary - on PPC, the 
> > values provided by GLIBC are 32-bit words.
> > 
> > Proposed comment describing this intrinsic:
> > ```
> > // This intrinsic is provided to allow back ends to emit load
> > // instructions that load a value from a fixed address. The
> > // parameter to the intrinsic is not an address, but an
> > // immediate index into an enumeration that contains the
> > // union of all such values available on all back ends.
> > // An example is the HWCAP/HWCAP2/CPUID words
> > // provided by GLIBC on PowerPC to allow fast access
> > // to commonly used parts of AUXV. These are provided
> > // at a fixed offset into the TCB (accessible through the
> > // thread pointer).
> > ```
> This is baking an a very target specific implementation of device 
> identification. Could you redefine this as something more abstract? Like 
> returns a device ID integer, or a bool that some int input is supported?
The idea is for this to not be restricted to device ID at all, but to be used 
for any values that reside in a fixed address for the compiler. For example, 
`STACK_GUARD` can be one of the values. How about if the intrinsic returns any 
integer type (or maybe any type)?
My thinking is that there may be need in the future for various things to be 
loaded from fixed addresses. The list of possible things that can be loaded 
this way would be a union of what all targets want and only specific values 
would make sense on each target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

@arsenm Has my description adequately addressed your question? Do you think we 
should move forward with [some version of] this patch or do you have any 
fundamental objections?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D152914#4428692 , @ilinpv wrote:

> Thank you for the patch, it comes in the right time - we are also working on 
> AArch64 __builtin_cpu_supports, and I was thinking how to make it more 
> general.
> I uploaded our RFC version for review https://reviews.llvm.org/D153153
>
>
> It would be great to have in __builtin_cpu_supports argument string of 
> plus-separated features. Just SemaBuiltinCpuSupports need to handle this.

Hi Pavel,
I took a quick look at your patch. I think it would be preferable to make the 
builtins target-independent rather than implementing the builtin by the same 
name for multiple targets. Although I think it is very useful to support a 
plus-separated list for `__builtin_cpu_supports()`, I think that's probably 
something for a subsequent patch. We would need to figure out code generation 
for that - perhaps that part will have to be completely target specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D155111: [clangd] Fix build failures observed on build bots for missing libs

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Lets get this committed to unblock the bots and if it is not the 
correct/desired fix, the author can subsequently provide the more appropriate 
fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155111

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

[PATCH] D154357: [Driver] Recognize powerpc-unknown-eabi as a bare-metal toolchain

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai accepted this revision.
nemanjai added a comment.

No concerns from the perspective of PowerPC here. Of course, my focus is 
primarily on the server side of things but I am not aware of any other group 
that could be adversely affected.


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

https://reviews.llvm.org/D154357

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

[PATCH] D106409: [PowerPC] Truncate exponent parameter for vec_cts,vec_ctf

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106409

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

arsenm wrote:
> From this description I don't understand what this is supposed to do. What 
> does the input mean? Why does this use an i32 immarg and not a pointer? Why 
> is the result only i32?
That is fair enough. The description is fairly vague. I can try to improve it 
as per below. The parameter for this is not a pointer (i.e. not an address). It 
is an immediate that represents the index into the enumeration of values that 
are provided at a fixed address. The back end is then free to produce the 
actual fixed address and the load itself.
The choice for the result type was admittedly arbitrary - on PPC, the values 
provided by GLIBC are 32-bit words.

Proposed comment describing this intrinsic:
```
// This intrinsic is provided to allow back ends to emit load
// instructions that load a value from a fixed address. The
// parameter to the intrinsic is not an address, but an
// immediate index into an enumeration that contains the
// union of all such values available on all back ends.
// An example is the HWCAP/HWCAP2/CPUID words
// provided by GLIBC on PowerPC to allow fast access
// to commonly used parts of AUXV. These are provided
// at a fixed offset into the TCB (accessible through the
// thread pointer).
```



Comment at: llvm/include/llvm/IR/Intrinsics.td:905
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],

arsenm wrote:
> I think a new intrinsic should be in a separate commit 
Sounds good.



Comment at: llvm/include/llvm/Support/TargetOpcodes.def:148
 
+/// This pseudo-instruction loads a value provided by libc at afixed address.
+/// For example, GLIBC provides HWCAP, HWCAP2 and CPUID at a fixed address

arsenm wrote:
> Typo afixed
ACK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D148697: [clang-tidy] Add more checks for functions which should be noexcept

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D148697#4422642 , @PiotrZSL wrote:

> In D148697#4422629 , @nemanjai 
> wrote:
>
>> It was also causing PPC bots to break. I fixed it in 
>> https://reviews.llvm.org/rGffd7a200fdfbd01ef296101647d2f2da91ddfd41
>
> O, thank you. Thats explain a lot. Because when I look into cmake, 
> clangTidyPerformanceModule were already there because you already added it.

Yeah, sorry. I saw that Phabricator automatically added the mention into the 
Diffusion link but I forgot that it won't add it to the Differential and that I 
need to add it manually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148697

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

[PATCH] D148697: [clang-tidy] Add more checks for functions which should be noexcept

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D148697#4421739 , @Dinistro wrote:

> In D148697#4421643 , @PiotrZSL 
> wrote:
>
>> @Dinistro It compiles & links with clang 16 also (shared libs). So I'm 
>> unable to reproduce.
>
> I used the following command to build:
>
>   cmake -S/home/christianu/repos/llvm-project/llvm 
> -B/home/christianu/repos/llvm-project/build-clang-tidy -GNinja 
> -DLLVM_ENABLE_LLD=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" 
> -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_ASSERTIONS=ON 
> -DBUILD_SHARED_LIBS=ON -DLLVM_INCLUDE_UTILS=ON -DLLVM_INSTALL_UTILS=ON 
> -DLLVM_BUILD_EXAMPLES=OFF -DLLVM_CCACHE_BUILD=ON -DLLVM_OPTIMIZED_TABLEGEN=ON 
> -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang 
> -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_RUNTIMES=openmp

It was also causing PPC bots to break. I fixed it in 
https://reviews.llvm.org/rGffd7a200fdfbd01ef296101647d2f2da91ddfd41


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148697

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1785
 
+  if (static_cast(TM).hasGlibcHWCAPAccess())
+OutStreamer->emitSymbolValue(

This probably deserves a comment along the lines of:
```
// Emit a reference to a symbol exported by versions of GLIBC
// that provide the HWCAP access. If a program built with this
// is run on a system with an old GLIBC, it should cause a
// load-time error rather than loading garbage.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai created this revision.
nemanjai added reviewers: PowerPC, RKSimon, pengfei, arsenm, t.p.northover.
Herald added subscribers: steven.zhang, kbarton, hiraditya.
Herald added a project: All.
nemanjai requested review of this revision.
Herald added subscribers: jdoerfert, wdng.
Herald added projects: clang, LLVM.

This is just a proposal patch for a possible direction we can extend these 
builtins to other targets as well as an implementation for PowerPC.

This adds a new instruction similar to `LOAD_STACK_GUARD` that is meant to load 
things from fixed addresses. The new instruction has semantics that are a 
superset of what `LOAD_STACK_GUARD` does so the two can be folded into one.

My hope with this patch is to collect feedback from the community about the 
direction, so please provide any feedback you may have.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152914

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/BuiltinsX86.def
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-cpu-supports.c
  clang/test/Sema/builtin-cpu-supports.c
  llvm/include/llvm/Analysis/TargetLibraryInfo.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/include/llvm/Support/TargetOpcodes.def
  llvm/include/llvm/Target/Target.td
  llvm/include/llvm/TargetParser/PPCTargetParser.def
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/lib/Target/PowerPC/PPCISelLowering.cpp
  llvm/lib/Target/PowerPC/PPCISelLowering.h
  llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
  llvm/lib/Target/PowerPC/PPCSubtarget.h
  llvm/lib/Target/PowerPC/PPCTargetMachine.h
  llvm/test/CodeGen/PowerPC/cpu-supports.ll

Index: llvm/test/CodeGen/PowerPC/cpu-supports.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/cpu-supports.ll
@@ -0,0 +1,111 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -mtriple=powerpc64-linux-gnu < %s | FileCheck  %s \
+; RUN:   -check-prefix=BE64
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -mtriple=powerpc-linux-gnu < %s | FileCheck  %s \
+; RUN:   -check-prefix=BE32
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -mtriple=powerpc64le-linux-gnu < %s | FileCheck  %s \
+; RUN:   -check-prefix=LE
+define dso_local signext i32 @test(i32 noundef signext %a) local_unnamed_addr #0 {
+; BE64-LABEL: test:
+; BE64:   # %bb.0: # %entry
+; BE64-NEXT:lwz r4, -28772(r13)
+; BE64-NEXT:andis. r4, r4, 128
+; BE64-NEXT:bne cr0, .LBB0_3
+; BE64-NEXT:  # %bb.1: # %if.else
+; BE64-NEXT:lwz r4, -28776(r13)
+; BE64-NEXT:andis. r4, r4, 1024
+; BE64-NEXT:bne cr0, .LBB0_4
+; BE64-NEXT:  # %bb.2: # %if.else3
+; BE64-NEXT:lwz r4, -28764(r13)
+; BE64-NEXT:cmplwi r4, 39
+; BE64-NEXT:addi r4, r3, 5
+; BE64-NEXT:slwi r3, r3, 1
+; BE64-NEXT:iseleq r3, r3, r4
+; BE64-NEXT:  .LBB0_3: # %return
+; BE64-NEXT:extsw r3, r3
+; BE64-NEXT:blr
+; BE64-NEXT:  .LBB0_4: # %if.then2
+; BE64-NEXT:addi r3, r3, -5
+; BE64-NEXT:extsw r3, r3
+; BE64-NEXT:blr
+;
+; BE32-LABEL: test:
+; BE32:   # %bb.0: # %entry
+; BE32-NEXT:lwz r4, -28732(r2)
+; BE32-NEXT:andis. r4, r4, 128
+; BE32-NEXT:bnelr cr0
+; BE32-NEXT:  # %bb.1: # %if.else
+; BE32-NEXT:lwz r4, -28736(r2)
+; BE32-NEXT:andis. r4, r4, 1024
+; BE32-NEXT:bne cr0, .LBB0_3
+; BE32-NEXT:  # %bb.2: # %if.else3
+; BE32-NEXT:lwz r4, -28724(r2)
+; BE32-NEXT:cmplwi r4, 39
+; BE32-NEXT:addi r4, r3, 5
+; BE32-NEXT:slwi r3, r3, 1
+; BE32-NEXT:iseleq r3, r3, r4
+; BE32-NEXT:blr
+; BE32-NEXT:  .LBB0_3: # %if.then2
+; BE32-NEXT:addi r3, r3, -5
+; BE32-NEXT:blr
+;
+; LE-LABEL: test:
+; LE:   # %bb.0: # %entry
+; LE-NEXT:lwz r4, -28776(r13)
+; LE-NEXT:andis. r4, r4, 128
+; LE-NEXT:bne cr0, .LBB0_3
+; LE-NEXT:  # %bb.1: # %if.else
+; LE-NEXT:lwz r4, -28772(r13)
+; LE-NEXT:andis. r4, r4, 1024
+; LE-NEXT:bne cr0, .LBB0_4
+; LE-NEXT:  # %bb.2: # %if.else3
+; LE-NEXT:lwz r4, -28764(r13)
+; LE-NEXT:cmplwi r4, 39
+; LE-NEXT:addi r4, r3, 5
+; LE-NEXT:slwi r3, r3, 1
+; LE-NEXT:iseleq r3, r3, r4
+; LE-NEXT:  .LBB0_3: # %return
+; LE-NEXT:extsw r3, r3
+; LE-NEXT:blr
+; LE-NEXT:  .LBB0_4: # %if.then2
+; LE-NEXT:addi r3, r3, -5
+; LE-NEXT:extsw r3, r3
+; LE-NEXT:blr
+entry:
+  %cpu_supports = tail call i32 @llvm.fixed.addr.ld(i32 1)
+  %0 = and i32 %cpu_supports, 8388608
+  %.not = icmp eq i32 %0, 0
+  br i1 %.not, label %if.else, label %return
+
+if.else:  ; preds = %entry
+  %cpu_supports1 = tail call 

[PATCH] D143467: [PowerPC] Add target feature requirement to builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai accepted this revision.
nemanjai added subscribers: maryammo, kamaub, stefanp.
nemanjai added a comment.
This revision is now accepted and ready to land.

This looks fine to me. I'd like some of the devs that have added builtins with 
Sema checking in the past to have a look here as well. @amyk @maryammo @kamaub 
@stefanp Can you have a look at this as well please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143467

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

[PATCH] D135171: FreeBSD: enable __float128 on x86 and powerpc64le

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai accepted this revision.
nemanjai added a comment.

My comments are minor nits that don't require another review, so LGTM.




Comment at: clang/lib/Basic/Targets/OSTargets.h:245-250
 switch (Triple.getArch()) {
-default:
+case llvm::Triple::ppc64le:
 case llvm::Triple::x86:
 case llvm::Triple::x86_64:
+  this->HasFloat128 = true;
+}

Seems a bit odd to have a separate switch on the same value for `__float128` 
rather than using fallthrough.



Comment at: clang/test/CodeGenCXX/float128-declarations.cpp:5-6
 // RUN:   -target-feature +float128 -std=c++11 %s -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-freebsd -std=c++11 \
+// RUN:   %s -o - | FileCheck %s -check-prefix=CHECK-X86
 // RUN: %clang_cc1 -emit-llvm -triple i386-unknown-linux-gnu -std=c++11 \

Now that you added `ppc64le`, did you mean to also add a line with that triple 
without the explicit `-target-feature`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135171

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

[PATCH] D143467: [PowerPC] Add target feature requirement to builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Actually, while you're here, can you please remove all calls to 
`SemaFeatureCheck()` that test builtins which are only available on a specific 
CPU? The Sema checking happens too early before the target info is populated, 
which does not allow for a very common idiom where the feature is enabled for a 
function/region using an attribute or a pragma.

For example, we received a report from Eigen that something like this:

  __attribute__((target("mma,prefix-instrs")))
  void test5(unsigned char *vqp, unsigned char *vpp, vector unsigned char vc,
 unsigned char *resp) {
__vector_quad vq = *((__vector_quad *)vqp);
__vector_pair vp = *((__vector_pair *)vpp);
__builtin_mma_pmxvf64ger(, vp, vc, 0, 0);
*((__vector_quad *)resp) = vq;
  }
  
  vector unsigned char test6(vector unsigned char a, vector unsigned char b) {
return a + b + (vector unsigned char)121;
  }

Cannot be done with Clang because it produces errors such as:

  error: this builtin is only valid on POWER10 or later CPUs
__builtin_mma_pmxvf64ger(, vp, vc, 0, 0);
^~~




Comment at: clang/include/clang/Basic/BuiltinsPPC.def:83-84
 BUILTIN(__builtin_ppc_lwarx, "iiD*", "")
-BUILTIN(__builtin_ppc_lharx, "ssD*", "")
-BUILTIN(__builtin_ppc_lbarx, "ccD*", "")
+TARGET_BUILTIN(__builtin_ppc_lharx, "ssD*", "", "isa-v206-instructions")
+TARGET_BUILTIN(__builtin_ppc_lbarx, "ccD*", "", "isa-v206-instructions")
 BUILTIN(__builtin_ppc_stdcx, "iLiD*Li", "")

Please change this to 207.



Comment at: clang/include/clang/Basic/BuiltinsPPC.def:87-88
 BUILTIN(__builtin_ppc_stwcx, "iiD*i", "")
-BUILTIN(__builtin_ppc_sthcx, "isD*s", "")
-BUILTIN(__builtin_ppc_stbcx, "icD*i", "")
+TARGET_BUILTIN(__builtin_ppc_sthcx, "isD*s", "", "isa-v206-instructions")
+TARGET_BUILTIN(__builtin_ppc_stbcx, "icD*i", "", "isa-v206-instructions")
 BUILTIN(__builtin_ppc_tdw, "vLLiLLiIUi", "")

Similarly to the loads.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143467

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

[PATCH] D143467: [PowerPC] Add target feature requirement to builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D143467#4195474 , @qiucf wrote:

> - For builtins with matching instruction, use the required ISA/vector version
> - For the rest builtins used in altivec.h, use requirements specified by the 
> header
> - Keep the feature checks in SemaChecking as-is, since they give user useful 
> message (`only available on POWER8 or later CPUs` instead of `requires 
> isa-v207-instructions to be enabled`)
>   - `lharx` and similar instructions exist since ISA v2.06 (Power 7), while 
> SemaChecking.cpp requires ISA v2.07 (Power 8)

There are quite a number of Power7 CPU's that do not have `l[bh]arx`. These 
were a late addition (i.e. in 2.06b). We should only emit it on Power8.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143467

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

[PATCH] D143467: [PowerPC] Add target feature requirement to builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Another note regarding the motivating test case:
Use of `vector double` should require VSX. We don't really seem to have the 
ability to turn this off early enough to catch this though. It would seem that 
in the front end, the target features depend on the compilation options and not 
the function itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143467

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

[PATCH] D143467: [PowerPC] Add target feature requirement to builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/include/clang/Basic/BuiltinsPPC.def:491
+TARGET_BUILTIN(__builtin_altivec_vabsduh, "V8UsV8UsV8Us", "", "altivec")
+TARGET_BUILTIN(__builtin_altivec_vabsduw, "V4UiV4UiV4Ui", "", "altivec")
 

qiucf wrote:
> shchenz wrote:
> > These builtins `vabsdub`, `vabsduh`, `vabsduw`  should require ISA3.0 which 
> > is not altivec or vsx. Do we have a reasonable feature for Power9 
> > instructions, `power9-vector` maybe?
> Thanks. I'll update then. `power9-vector` is good option. But I'm curious 
> what's the different pratical usages from `power9-vector` and 
> `isa-v30-instructions`. Maybe cc @nemanjai 
We don't want ISA 3.0 to imply that it is OK to use vector instructions. For 
example, most of the distros are now Power9 and up. So the kernel is built with 
`-mcpu=power9`. However, the kernel is also built with `-mno-altivec` so we 
don't want to allow any use of vector instructions/registers in the kernel 
while we want ISA 3.0 instructions.
And that's just one use case. Ultimately, we keep these separate so we can 
control scalar ISA instructions independently of the vector ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143467

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

[PATCH] D145141: [Driver] Reject -march= for ppc

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

I am perfectly fine with this. That option is quite problematic. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145141

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

[PATCH] D144967: [PowerPC] Recognize long CPU name for -mtune in Clang

[Nemanja Ivanovic via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG59cd692454c9: [PowerPC] Recognize long CPU name for -mtune 
in Clang (authored by nemanjai).

Changed prior to commit:
  https://reviews.llvm.org/D144967?vs=501133=501934#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144967

Files:
  clang/lib/Driver/ToolChains/Arch/PPC.cpp
  clang/lib/Driver/ToolChains/Arch/PPC.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/ppc-cpus.c

Index: clang/test/Driver/ppc-cpus.c
===
--- clang/test/Driver/ppc-cpus.c
+++ clang/test/Driver/ppc-cpus.c
@@ -31,6 +31,8 @@
 
 // RUN: %clang -### -c --target=powerpc64 %s -mcpu=generic -mtune=pwr9 2>&1 | FileCheck %s --check-prefix=TUNE
 // TUNE: "-target-cpu" "ppc64" "-tune-cpu" "pwr9"
+// RUN: %clang -### -c --target=powerpc64le %s -mcpu=power9 -mtune=power10 2>&1 | FileCheck %s --check-prefix=TUNE-LONG
+// TUNE-LONG: "-target-cpu" "pwr9" "-tune-cpu" "pwr10"
 
 /// Test mcpu options that are equivalent to "generic"
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=generic 2>&1 | FileCheck %s --check-prefix=GENERIC
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1992,17 +1992,15 @@
 
 void Clang::AddPPCTargetArgs(const ArgList ,
  ArgStringList ) const {
+  const llvm::Triple  = getToolChain().getTriple();
   if (const Arg *A = Args.getLastArg(options::OPT_mtune_EQ)) {
 CmdArgs.push_back("-tune-cpu");
-if (strcmp(A->getValue(), "native") == 0)
-  CmdArgs.push_back(Args.MakeArgString(llvm::sys::getHostCPUName()));
-else
-  CmdArgs.push_back(A->getValue());
+std::string CPU = ppc::getPPCTuneCPU(Args, T);
+CmdArgs.push_back(Args.MakeArgString(CPU));
   }
 
   // Select the ABI to use.
   const char *ABIName = nullptr;
-  const llvm::Triple  = getToolChain().getTriple();
   if (T.isOSBinFormatELF()) {
 switch (getToolChain().getArch()) {
 case llvm::Triple::ppc64: {
Index: clang/lib/Driver/ToolChains/Arch/PPC.h
===
--- clang/lib/Driver/ToolChains/Arch/PPC.h
+++ clang/lib/Driver/ToolChains/Arch/PPC.h
@@ -37,6 +37,8 @@
 
 std::string getPPCTargetCPU(const llvm::opt::ArgList ,
 const llvm::Triple );
+std::string getPPCTuneCPU(const llvm::opt::ArgList ,
+  const llvm::Triple );
 const char *getPPCAsmModeForCPU(StringRef Name);
 ReadGOTPtrMode getPPCReadGOTPtrMode(const Driver , const llvm::Triple ,
 const llvm::opt::ArgList );
Index: clang/lib/Driver/ToolChains/Arch/PPC.cpp
===
--- clang/lib/Driver/ToolChains/Arch/PPC.cpp
+++ clang/lib/Driver/ToolChains/Arch/PPC.cpp
@@ -34,53 +34,60 @@
 return "ppc";
 }
 
-/// getPPCTargetCPU - Get the (LLVM) name of the PowerPC cpu we are targeting.
-std::string ppc::getPPCTargetCPU(const ArgList , const llvm::Triple ) {
-  if (Arg *A = Args.getLastArg(clang::driver::options::OPT_mcpu_EQ)) {
-StringRef CPUName = A->getValue();
-
-// Clang/LLVM does not actually support code generation
-// for the 405 CPU. However, there are uses of this CPU ID
-// in projects that previously used GCC and rely on Clang
-// accepting it. Clang has always ignored it and passed the
-// generic CPU ID to the back end.
-if (CPUName == "generic" || CPUName == "405")
+static std::string normalizeCPUName(StringRef CPUName, const llvm::Triple ) {
+  // Clang/LLVM does not actually support code generation
+  // for the 405 CPU. However, there are uses of this CPU ID
+  // in projects that previously used GCC and rely on Clang
+  // accepting it. Clang has always ignored it and passed the
+  // generic CPU ID to the back end.
+  if (CPUName == "generic" || CPUName == "405")
+return getPPCGenericTargetCPU(T);
+
+  if (CPUName == "native") {
+std::string CPU = std::string(llvm::sys::getHostCPUName());
+if (!CPU.empty() && CPU != "generic")
+  return CPU;
+else
   return getPPCGenericTargetCPU(T);
+  }
 
-if (CPUName == "native") {
-  std::string CPU = std::string(llvm::sys::getHostCPUName());
-  if (!CPU.empty() && CPU != "generic")
-return CPU;
-  else
-return getPPCGenericTargetCPU(T);
-}
+  return llvm::StringSwitch(CPUName)
+  .Case("common", "generic")
+  .Case("440fp", "440")
+  .Case("630", "pwr3")
+  .Case("G3", "g3")
+  .Case("G4", "g4")
+  .Case("G4+", "g4+")
+  .Case("8548", "e500")
+  .Case("G5", "g5")
+  .Case("power3", "pwr3")
+  .Case("power4", "pwr4")
+  

[PATCH] D144967: [PowerPC] Recognize long CPU name for -mtune in Clang

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D144967#4165140 , @nemanjai wrote:

> I'll provide an updated patch in a few min...

Ah, what I mean is I will update the patch and push in a few min.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144967

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

[PATCH] D144967: [PowerPC] Recognize long CPU name for -mtune in Clang

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

I'll provide an updated patch in a few min...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144967

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

[PATCH] D144967: [PowerPC] Recognize long CPU name for -mtune in Clang

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai created this revision.
nemanjai added reviewers: PowerPC, nathanchance, MaskRay.
Herald added subscribers: steven.zhang, shchenz, kbarton.
Herald added a project: All.
nemanjai requested review of this revision.
Herald added a project: clang.

There are two ways of specifying a CPU on PowerPC: `power` and `pwr`. 
Clang/LLVM traditionally supports the latter and Clang replaces the former with 
the latter when passing it to the back end for the `-mcpu=` option. However, 
when the `-mtune=` option was introduced, this replacement was not implemented 
for it.

This leaves us in an inconsistent state of accepting both forms for `-mcpu=` 
and and only the latter for `-mtune=`. Furthermore, it leaves us incompatible 
with GCC which only understands the `power` version for both options.

This patch just adds the same handling for the long names for `-mtune=` as 
already exists for `-mcpu=`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144967

Files:
  clang/lib/Driver/ToolChains/Arch/PPC.cpp
  clang/lib/Driver/ToolChains/Arch/PPC.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/ppc-cpus.c

Index: clang/test/Driver/ppc-cpus.c
===
--- clang/test/Driver/ppc-cpus.c
+++ clang/test/Driver/ppc-cpus.c
@@ -31,6 +31,8 @@
 
 // RUN: %clang -### -c --target=powerpc64 %s -mcpu=generic -mtune=pwr9 2>&1 | FileCheck %s --check-prefix=TUNE
 // TUNE: "-target-cpu" "ppc64" "-tune-cpu" "pwr9"
+// RUN: %clang -### -c --target=powerpc64le %s -mcpu=power9 -mtune=power10 2>&1 | FileCheck %s --check-prefix=TUNE-LONG
+// TUNE-LONG: "-target-cpu" "pwr9" "-tune-cpu" "pwr10"
 
 /// Test mcpu options that are equivalent to "generic"
 // RUN: %clang -### -c -target powerpc64 %s -mcpu=generic 2>&1 | FileCheck %s --check-prefix=GENERIC
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1992,17 +1992,15 @@
 
 void Clang::AddPPCTargetArgs(const ArgList ,
  ArgStringList ) const {
+  const llvm::Triple  = getToolChain().getTriple();
   if (const Arg *A = Args.getLastArg(options::OPT_mtune_EQ)) {
 CmdArgs.push_back("-tune-cpu");
-if (strcmp(A->getValue(), "native") == 0)
-  CmdArgs.push_back(Args.MakeArgString(llvm::sys::getHostCPUName()));
-else
-  CmdArgs.push_back(A->getValue());
+std::string CPU = ppc::getPPCTuneCPU(Args, T);
+CmdArgs.push_back(Args.MakeArgString(CPU));
   }
 
   // Select the ABI to use.
   const char *ABIName = nullptr;
-  const llvm::Triple  = getToolChain().getTriple();
   if (T.isOSBinFormatELF()) {
 switch (getToolChain().getArch()) {
 case llvm::Triple::ppc64: {
Index: clang/lib/Driver/ToolChains/Arch/PPC.h
===
--- clang/lib/Driver/ToolChains/Arch/PPC.h
+++ clang/lib/Driver/ToolChains/Arch/PPC.h
@@ -37,6 +37,8 @@
 
 std::string getPPCTargetCPU(const llvm::opt::ArgList ,
 const llvm::Triple );
+std::string getPPCTuneCPU(const llvm::opt::ArgList ,
+  const llvm::Triple );
 const char *getPPCAsmModeForCPU(StringRef Name);
 ReadGOTPtrMode getPPCReadGOTPtrMode(const Driver , const llvm::Triple ,
 const llvm::opt::ArgList );
Index: clang/lib/Driver/ToolChains/Arch/PPC.cpp
===
--- clang/lib/Driver/ToolChains/Arch/PPC.cpp
+++ clang/lib/Driver/ToolChains/Arch/PPC.cpp
@@ -34,53 +34,60 @@
 return "ppc";
 }
 
-/// getPPCTargetCPU - Get the (LLVM) name of the PowerPC cpu we are targeting.
-std::string ppc::getPPCTargetCPU(const ArgList , const llvm::Triple ) {
-  if (Arg *A = Args.getLastArg(clang::driver::options::OPT_mcpu_EQ)) {
-StringRef CPUName = A->getValue();
-
-// Clang/LLVM does not actually support code generation
-// for the 405 CPU. However, there are uses of this CPU ID
-// in projects that previously used GCC and rely on Clang
-// accepting it. Clang has always ignored it and passed the
-// generic CPU ID to the back end.
-if (CPUName == "generic" || CPUName == "405")
+static std::string getLLVMPPCCpuName(StringRef CPUName, const llvm::Triple ) {
+  // Clang/LLVM does not actually support code generation
+  // for the 405 CPU. However, there are uses of this CPU ID
+  // in projects that previously used GCC and rely on Clang
+  // accepting it. Clang has always ignored it and passed the
+  // generic CPU ID to the back end.
+  if (CPUName == "generic" || CPUName == "405")
+return getPPCGenericTargetCPU(T);
+
+  if (CPUName == "native") {
+std::string CPU = std::string(llvm::sys::getHostCPUName());
+if (!CPU.empty() && CPU != "generic")
+  return CPU;
+else
   return getPPCGenericTargetCPU(T);
+  }
 
-if (CPUName == "native") {
-  

[PATCH] D144293: [PowerPC] Fix the implicit casting for the emulated intrinsics

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM as long as the naming nit is addressed.




Comment at: clang/lib/Headers/ppc_wrappers/emmintrin.h:57
 typedef __vector unsigned char __v16qu;
+typedef __vector float __v2f;
 

The name `__v2f` seems strange since `__vector float` is a vector of 4 `float` 
values. Should this be `__v4f`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144293

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

[PATCH] D144321: [PowerPC] Correctly use ELFv2 ABI on all OS's that use the ELFv2 ABI

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai accepted this revision.
nemanjai added a comment.

Thanks for doing this. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144321

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

[PATCH] D144321: [PowerPC] Correctly use ELFv2 ABI on all OS's that use the ELFv2 ABI

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.h:430-432
+  if ((Triple.isOSFreeBSD() && (Triple.getOSVersion().empty() ||
+Triple.getOSMajorVersion() >= 13)) ||
+  Triple.isOSOpenBSD() || Triple.isMusl())

I am generally not a fan of keeping multiple instances of a somewhat complex 
conditions in sync. I wonder if the community would be OK with one of:
- Adding a static function in `Triple.h` to test this (`static bool 
isPPCELFv2ABI(const Triple &)`)
- Adding a member function `Triple::isPPCELFv2ABI()` (similar to 
`Triple::isWatchABI()`)

Then of course, we can use that function in both places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144321

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

[PATCH] D142222: [PowerPC] Remove the lax warning for explicit casts

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM other than the missing template test.




Comment at: clang/test/Parser/lax-conv.cpp:4
+// RUN: %clang_cc1 -triple=powerpc64-ibm-aix -target-feature +altivec 
-target-feature -vsx -fsyntax-only -verify=expected,aix %s
+
+void dummy(vector unsigned int a);

Can you also add one template test?
```
template  VEC __attribute__((noinline)) test(vector unsigned char 
a, vector unsigned char b) {
  return (VEC)(a * b);
}

vector unsigned char test1(vector unsigned char a, vector unsigned char b) {
  return test(a, b);
}

vector unsigned long long test2(vector unsigned char a, vector unsigned char b) 
{
  return test(a, b);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D14

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

[PATCH] D140080: [clang][PPC] Supporting -mcpu=405

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

I don't think the comments require another round of review. Thanks for fixing 
this.




Comment at: clang/lib/Driver/ToolChains/Arch/PPC.cpp:23-29
+static constexpr llvm::StringLiteral GenericCPUNames[] = {{"generic"}, 
{"405"}};
+
+/// Check if Name is known to be equivalent to "generic".
+static bool isGenericCPUName(const StringRef ) {
+  return llvm::is_contained(GenericCPUNames, Name);
+}
+

While I understand the motivation here, I think that this is overkill. We in 
fact hope not to have to grow the set of CPU ID's that we treat as generic.



Comment at: clang/lib/Driver/ToolChains/Arch/PPC.cpp:42
 
-if (CPUName == "generic")
   return getPPCGenericTargetCPU(T);

I think we should change this to check for `generic` or `405` and add a comment 
similar to the following:
```
// Clang/LLVM does not actually support code generation
// for the 405 CPU. However, there are uses of this CPU ID
// in projects that previously used GCC and rely on Clang
// accepting it. Clang has always ignored it and passed the
// generic CPU ID to the back end.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140080

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

[PATCH] D137511: [PPC] Undefine __ppc64__ to match GCC

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM.
Thanks so much for fixing up the existing code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137511

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

[PATCH] D137511: [PPC] Undefine __ppc64__ to match GCC

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

I am going to block this since the uses of the macros in 
`clang/lib/Headers/ppc_wrappers` will likely cause build bot failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137511

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

[PATCH] D137511: [PPC] Undefine __ppc64__ to match GCC

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

As mentioned by @q66 above, this can't go in until usage of this macro in the 
Clang/LLVM codebase is fixed. Looks like the uses in 
`clang/lib/Headers/ppc_wrappers` and some of the uses in `libcxx` and 
`libunwind` must be fixed while others should probably also be fixed to avoid 
confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137511

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

[PATCH] D134371: [clang-doc] Add typedef/using information.

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

This causes failures with `-Werror` such as: 
https://lab.llvm.org/buildbot/#/builders/57/builds/22322
Please fix or revert.




Comment at: clang-tools-extra/clang-doc/Representation.h:45
+  IT_enum,
+  IT_typedef
 };

There are at least a couple of `switch` statements where this enumerator isn't 
covered.
Examples:
```
clang-tools-extra/clang-doc/HTMLGenerator.cpp:847:11:
clang-tools-extra/clang-doc/MDGenerator.cpp:327:15
clang-tools-extra/clang-doc/MDGenerator.cpp:365:11
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134371

___
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

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

I am not crazy about adding the Boolean parameter here or about the name. Seems 
somewhat unclear when a caller wants to pass `true` there.

What I think would be a more robust solution would be to use the same logic 
that decides whether to coerce the struct argument to an integer type. It seems 
that any big endian ABI that does this would want to ensure the access is on 
the right side.

Ultimately what I am getting at here is that we consider how the caller passes 
the value and how the callee accesses it separately - which is what leads to 
problems like this. Can we decide using the same function for the caller and 
the callee?


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] D85599: [PowerPC] Remove isTerminator for TRAP instruction

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.
Herald added a project: All.

I think we should handle this similarly to `SITargetLowering::splitKillBlock()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85599

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

[PATCH] D129016: [PowerPC] implemented @llvm.ppc.kill.canary to corrupt stack guard

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129016

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

[PATCH] D129016: [PowerPC] implemented @llvm.ppc.kill.canary to corrupt stack guard

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10702
+
+// If SafeStack or !StackProtector, kill_canary is not supported.
+if (MF.getFunction().hasFnAttribute(Attribute::SafeStack) ||

Again, this comment is nothing more than a re-reading of the code. As such, it 
is not useful. The comment should say what is happening and why. Something 
along the lines of:
```
// The kill_canary intrinsic only makes sense when the Stack Protector
// feature is on in the function. It can also not be used in conjunction
// with safe stack because the latter splits the stack and the canary
// value isn't used (i.e. safe stack supersedes stack protector).
// In situations where the kill_canary intrinsic is not supported,
// we simply replace uses of its chain with its input chain, causing
// the SDAG CSE to remove the node.
```



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10720-10749
+if (useLoadStackGuardNode()) {
+  MachineSDNode *LSG =
+  DAG.getMachineNode(PPC::LOAD_STACK_GUARD, DL, VT, Op->getOperand(0));
+  Load = SDValue(LSG, 0);
+
+  // Frame index used to determine stack guard location if
+  // LOAD_STACK_GUARD is used.

The common code should just be common. Seems like the only thing that changes 
is how we load the value. Please refactor this to something like:
```
if (useLoadStackGuardNode()) {
  ...
  Load = ... // stack guard load
} else if (Value *GV = getSDagStackGuard(*M)) {
  ...
  Load = ... // canary word global load
} else
  llvm_unreachable("Unhandled stack guard case");

Store = ... // common store
return Store; // (or you can just create the store in-line and return it 
directly)
```



Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-aix 
-ppc-vsr-nums-as-vr \
+; RUN:   -mcpu=pwr7 --ppc-asm-full-reg-names < %s | FileCheck %s 
-check-prefix=CHECK-AIX

Please add one RUN line with `-O0` to ensure that this works as expected with 
Fast ISel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129016

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

[PATCH] D131346: [clang] LLVM_FALLTHROUGH => [[fallthrough]]. NFC

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Why? There are many years of precedent for using `LLVM_FALLTHROUGH` and it is 
very clear and obvious. What do we gain by getting rid of it?
Don't get me wrong, I am not super opposed to using a standard string instead 
of an LLVM-specific macro. However, it seems that this leaves us with a mixture 
of the macro and the standard attribute. If we are ready to replace all 
occurrences in all projects and get rid of the macro altogether (with some 
warning to downstream users), that seems reasonable. Replacing only some of 
them seems worse than what we now have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131346

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

[PATCH] D130526: [Driver][PowerPC] Support -mtune=

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Thanks. We'll eventually start doing something with the option in the back end. 
LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130526

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

[PATCH] D129855: [clang][PowerPC] Set lld as clang's default linker for PowerPC Linux

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D129855#3667191 , @MaskRay wrote:

> In D129855#3662457 , @quinnp wrote:
>
>> In D129855#3657006 , @MaskRay 
>> wrote:
>>
>>> This is not right as using `ld.lld` as the default linker isn't the 
>>> majority case. If you want to change the default for your distribution, set 
>>> `-DCLANG_DEFAULT_LINKER=lld`.
>>> (Alternatively, you can have a `ld` symlink pointing to `lld`.)
>>
>> Hi @MaskRay! Do you mean I should abandon this change or find a way to set 
>> the CMake variable `CLANG_DEFAULT_LINKER` to `lld` as default when building 
>> for PowerPC Linux? I wasn't able to find any examples of people setting 
>> CMake variables for specific distributions.
>>
>> Thanks!
>
> You can customize `CLANG_DEFAULT_LINKER` in your clang distribution. I don't 
> find convincing argument to change the default for `PPCLinuxToolChain` and 
> diverge from `Linux`.

The reason we would like the default linker to be `ld.lld` for most/default 
builds on PPC is because using LTO without the GPL-licensed Gold plugin 
requires LLD. The idea is that a typical user can pull the source and build it 
with minimal CMake macros and get a working LTO without having to build the 
Gold plugin.

Of course, this may not be the way to accomplish this (i.e. this will make it 
diverge from the value specified in `CLANG_DEFAULT_LINKER` in 
`$LLVM_BUILD/tools/clang/include/clang/Config/config.h`). So I would prefer 
that we handle this in the CMake files if @MaskRay doesn't object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129855

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

[PATCH] D129016: [PowerPC] implemented @llvm.ppc.kill.canary to corrupt stack guard

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Please run clang-format, rebase and re-upload. It doesn't apply cleanly to ToT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129016

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

[PATCH] D128652: [PowerPC] Finished kill_canary implementation and debugging

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

"Made a new phabricator review because of git issues" is not an appropriate 
description of a review/revision.

Hopefully the description you add will describe what this intrinsic is supposed 
to do. It seems to me that this is a poorly designed feature if it is meant to 
work the way it was implemented. Namely, it seems like this intrinsic clobbers 
the stack protect global value rather than clobbering the corresponding value 
on the stack for the specific function it is enclosed in. I would have thought 
that it will clobber the stack in the function, thereby allowing stack 
protection to work as expected for other functions in the module.




Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5014
+
+if (IntrinsicID == Intrinsic::ppc_kill_canary) {
+  CurDAG->SelectNodeTo(N, PPC::NOP, MVT::Other, N->getOperand(0));

I think it would be preferable to handle this intrinsic in one place. The `nop` 
is not actually necessary here. We should simply remove the intrinsic from the 
stream in `PPCISelLowering.cpp` and not pass it on.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11132
+  case Intrinsic::ppc_kill_canary: { 
+MachineFunction  = DAG.getMachineFunction();
+if (MF.getFunction().hasFnAttribute(Attribute::SafeStack) ||

The formatting of this entire block is quite messed up. Please run 
`clang-format` on this.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11138
+
+IRBuilder<> B(().getEntryBlock().front());
+

Do we use this?



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11144
+
+if (GV == nullptr) break;
+EVT VT = DAG.getTargetLoweringInfo().getValueType(DAG.getDataLayout(), 
GV->getType(), true); 

Is it ok to just ignore a failure to get the GV here? Should this not be an 
assert?



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11156-11168
+SDValue Store = DAG.getStore( 
+Op->getOperand(0), 
+DL,
+   DAG.getNode(
+ISD::XOR,
+   DL,
+   VT,

What is happening here? We load the value, XOR it with itself, store it again? 
Isn't that just zeroing it out? Why do we even need to load it then?



Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
+; RUN:   --ppc-asm-full-reg-names < %s | FileCheck %s

At the very least, this has to also include a RUN line for Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128652

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

[PATCH] D127189: [clang][AIX] Add option to control quadword lock free atomics ABI on AIX

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

I am ok with this change overall, I just have a couple of questions about 
naming of the option.

1. Is there any precedent for options that start with `-maix` or `-m` for 
any other OS?
2. Is `quadword` the best word to use? There is no type information and this is 
restricted to integers. Would something like `-maix-i128-atomics` be a better 
name?
3. Since this is kind of an ABI-related decision, would it make sense (and 
would it be possible) to make this a further suboption to the `-mabi` option? 
Something like `-mabi=vec-extabi,i128-atomics,ieeelongdouble`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127189

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

[PATCH] D124093: [PowerPC] Fixing implicit castings in altivec for -fno-lax-vector-conversions

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM. Thanks.




Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c:8
 // RUN:   -D__XL_COMPAT_ALTIVEC__ -target-cpu pwr8 | FileCheck %s
-// RUN: %clang_cc1 -no-opaque-pointers -target-feature +altivec 
-target-feature +vsx \
+// RUN: %clang_cc1 -flax-vector-conversions=none -no-opaque-pointers 
-target-feature +altivec -target-feature +vsx \
 // RUN:   -triple powerpc64le-unknown-linux-gnu -emit-llvm %s -o - \

lei wrote:
> nemanjai wrote:
> > I don't know why only one of the functions below has checks for this run 
> > line, but that needs to be fixed. Please add the `NOCOMPAT` checks on the 
> > other functions.
> Can we address this on a followup patch? Since it's not related to the work 
> here.
Absolutely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124093

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

[PATCH] D126540: PowerPC] Emit warning for incompatible vector types that are currently diagnosed with -fno-lax-vector-conversions

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7571-7573
+  "Current bitcast for incompatible vector types (%0 and %1) are deprecated. "
+  "The default behaviour will change to what is implied by the "
+  "-fno-lax-vector-conversions option">,

We should not mention the word `bitcast` in the message as it doesn't really 
mean anything to C/C++ developers not used to LLVM IR.
We can make the text something like:
```
Implicit conversion between vector types ('%0' and '%1') is deprecated. In the 
future, the behavior implied by '-fno-lax-vector-conversions' will be the 
default.
```



Comment at: clang/lib/Sema/SemaExpr.cpp:7718
+  assert((DestTy->isVectorType() || SrcTy->isVectorType()) &&
+ "areAnyVectorTypesAltivec - invalid input types");
+

The message does not need to mention the function name. Something like
`"expected at least one type to be a vector here"` should suffice.



Comment at: clang/lib/Sema/SemaExpr.cpp:7720
+
+  bool isSrcTyAltivec = false;
+  bool isDestTyAltivec = false;

Nit: naming convention. Here and elsewhere - please review all of your variable 
names.



Comment at: clang/lib/Sema/SemaExpr.cpp:7723-7727
+  if (SrcTy->isVectorType()) {
+VectorType::VectorKind SrcVecKind =
+SrcTy->castAs()->getVectorKind();
+isSrcTyAltivec = (SrcVecKind == VectorType::AltiVecVector);
+  }

maryammo wrote:
> maryammo wrote:
> > lei wrote:
> > > do we really need this check since we have an assert above?
> > Yes, without this check there is gonna be a compile time failure for cases 
> > where SrcTy is not vector but we wanna cast it to vector 
> > 'SrcTy->castAs()' 
> the assert checks of either of them is a vector but then we wanna check if at 
> least one of them is altivec. 
I think the whole thing can simply be:
```
bool IsSrcTyAltivec = SrcTy->isVectorType() &&
  (SrcTy->castAs()->getVectorKind() == VectorType::AltiVecVector);
```
And similarly for the destination type.



Comment at: clang/lib/Sema/SemaExpr.cpp:7740
+  assert((DestTy->isVectorType() || SrcTy->isVectorType()) &&
+ "areVectorTypesSameElmType -invalid input types");
+

Similarly to my comment above regarding the assert message.



Comment at: clang/lib/Sema/SemaExpr.cpp:9571
   if (isLaxVectorConversion(RHSType, LHSType)) {
+if (areAnyVectorTypesAltivec(RHSType, LHSType) &&
+!areVectorTypesSameElmType(RHSType, LHSType))

Please add a comment:
```
// The default for lax vector conversions with Altivec vectors will
// change, so if we are converting between vector types where
// at least one is an Altivec vector, emit a warning.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126540

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

[PATCH] D124093: [PowerPC] Fixing implicit castings in altivec for -fno-lax-vector-conversions

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15587
+
+bool isUnaligned = (BuiltinID == PPC::BI__builtin_altivec_vinsw ||
+BuiltinID == PPC::BI__builtin_altivec_vinsd)

This is strange. You don't need the ternary operator when you are simply 
setting a `bool` variable. This is simply:
```
bool isUnaligned = (BuiltinID == PPC::BI__builtin_altivec_vinsw ||
BuiltinID == PPC::BI__builtin_altivec_vinsd);
```
Same below.

Also, please be careful of the naming convention (i.e. variables start with 
upper case, functions with lowercase, etc.). I will not add further comments to 
this end but please apply this change to the entire patch.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15604
+// builtin called.
+int validMaxValue = isUnaligned ? is32bit ? 12 : 8 : is32bit ? 3 : 1;
+

This is certainly concise, but trying to parse it puts my brain into an 
infinite loop. Please write it as:
```
int ValidMaxValue = 0;
if (IsUnaligned)
  ValidMaxValue = ... ? ... : ...
else
  ValidMaxValue = ...
```



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15608
+std::string rangeErrMsg = isUnaligned ? "byte" : "element";
+rangeErrMsg += " number is outside of the valid range [0, ";
+rangeErrMsg += llvm::to_string(validMaxValue) + "]";

Since you are building a string from pieces here, might as well mention the 
value the user specified so it is something like:
`byte number 44 is outside of the valid range [0, 12]`



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15661
+Value *Op1 = EmitScalarExpr(E->getArg(1));
+llvm::Type *Int128Ty = llvm::IntegerType::get(getLLVMContext(), 128);
+Ops.push_back(

Why not simplify the bitcasts below by just creating a pointer to the vector 
type (call it say, `V1I128Ty`) here? Same in the next `case`.



Comment at: clang/lib/Headers/altivec.h:3211-3219
+   : (vector double)(__builtin_vsx_xvcvuxdsp(  
\
+ (vector unsigned long long)(__a)) *   
\
+ (vector float)(vector unsigned)((0x7f - (__b))
\
+ << 23)),  
\
  vector signed long long   
\
-   : (__builtin_vsx_xvcvsxdsp((vector signed long long)(__a)) *
\
-  (vector float)(vector unsigned)((0x7f - (__b)) << 23)))
+   : (vector double)(__builtin_vsx_xvcvsxdsp(  
\
+ (vector signed long long)(__a)) * 
\

This is wrong. Please see the documentation for what the result type should be 
under XL compat. I believe it needs to be `vector float` regardless of the 
input.



Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c:8
 // RUN:   -D__XL_COMPAT_ALTIVEC__ -target-cpu pwr8 | FileCheck %s
-// RUN: %clang_cc1 -no-opaque-pointers -target-feature +altivec 
-target-feature +vsx \
+// RUN: %clang_cc1 -flax-vector-conversions=none -no-opaque-pointers 
-target-feature +altivec -target-feature +vsx \
 // RUN:   -triple powerpc64le-unknown-linux-gnu -emit-llvm %s -o - \

I don't know why only one of the functions below has checks for this run line, 
but that needs to be fixed. Please add the `NOCOMPAT` checks on the other 
functions.



Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c:30
 // CHECK-NEXT:[[TMP1:%.*]] = call <4 x float> @llvm.ppc.vsx.xvcvsxdsp(<2 x 
i64> [[TMP0]])
 // CHECK-NEXT:fmul <4 x float> [[TMP1]], 
 

We compute a `vector float` but return a `vector double`. Something is wrong. 
Please see my comment regarding this in `altivec.h`.



Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:746
 [IntrNoMem, ImmArg>]>;
+
   // P10 Vector Extract.

What happened here? Unrelated whitespace change. Please remove it before 
committing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124093

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

[PATCH] D127310: [clang][driver] fix to correctly set devtoolset on RHEL

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D127310#3567472 , @MaskRay wrote:

> Do you have more authoritative answer when /root/usr is used and when it 
> isn't?

These suffixes were always part of the code and the mentioned changeset removed 
it without any justification. The burden of providing this answer should lie 
with the author of the change that removed this (i.e. @tbaeder). Here at IBM, 
we are not aware of any Redhat release that does not have this suffix. Namely, 
all of our RH PowerPC machines with all the toolset versions and distro 
versions we have available have the suffix. So we are ill equipped to answer 
this question. I assume that Timm is aware of situations where this isn't part 
of the path.

> I'd wish that we have comments for them.

I agree 100%. Timm, do you know the answer to this and if so, can you propose a 
comment to that end?

> This change also needs a unit test.

+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127310

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

[PATCH] D125862: [clang][driver] Add gcc-toolset/devtoolset 12 to prefixes

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added subscribers: quinnp, nemanjai.
nemanjai added a comment.

The original toolchain detection added `root/usr` to the paths which was 
certainly necessary on PowerPC. Since that suffix is removed in this patch, our 
Redhat buildbot is now broken 
(https://lab.llvm.org/buildbot/#/builders/57/builds/18577). @quinnp has posted 
an update to this at https://reviews.llvm.org/D127310.


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

https://reviews.llvm.org/D125862

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

[PATCH] D127310: [clang][driver] fix to correctly set devtoolset on RHEL

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2152
 ChosenToolsetVersion = ToolsetVersion;
-ChosenToolsetDir = "/opt/rh/" + ToolsetDir.str();
+ChosenToolsetDir = "/opt/rh/" + ToolsetDir.str() + "/root/usr";
   }

I believe this will cause a failure with the unit test added in the original 
patch.

And presumably since the unit test tests for paths that don't include 
`/root/usr`, there are real world toolsets that also don't use that suffix. So 
we should presumably add both `Prefixes`.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2157
 if (ChosenToolsetVersion > 0)
   Prefixes.push_back(ChosenToolsetDir);
   }

Can we not just change this to something like:
```
if (ChosenToolsetVersion > 0) {
  Prefixes.push_back(ChosenToolsetDir);
  Prefixes.push_back(ChosenToolsetDir + "/root/usr");
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127310

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

[PATCH] D126302: [PowerPC] Diagnose invalid combination with Altivec, VSX and soft-float

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai created this revision.
nemanjai added reviewers: Laurentiu, PowerPC.
Herald added subscribers: shchenz, kbarton.
Herald added a project: All.
nemanjai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The current behaviour with these three options is quite undesirable:
`-mno-altivec -mvsx` allows VSX to override no Altivec, thereby turning on both
`-msoft-float -maltivec` causes a crash if an actual Altivec instruction is 
required because soft float turns of Altivec
`-msoft-float -mvsx` is also accepted with both Altivec and VSX turned off 
(potentially causing crashes as above)

This patch diagnoses these impossible combinations in the driver so the user 
does not end up with surprises in terms of their options being ignored or 
silently overridden.

Inspired by https://github.com/llvm/llvm-project/issues/6


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126302

Files:
  clang/lib/Basic/Targets/PPC.cpp
  clang/test/CodeGen/PowerPC/attr-target-ppc.c
  clang/test/Driver/ppc-dependent-options.cpp


Index: clang/test/Driver/ppc-dependent-options.cpp
===
--- clang/test/Driver/ppc-dependent-options.cpp
+++ clang/test/Driver/ppc-dependent-options.cpp
@@ -78,6 +78,14 @@
 // RUN: -mcpu=power10 -std=c++11 -mno-vsx -mpower10-vector %s 2>&1 | \
 // RUN: FileCheck %s -check-prefix=CHECK-NVSX-P10V
 
+// RUN: not %clang -target powerpc64le-unknown-unknown -fsyntax-only \
+// RUN: -std=c++11 -mvsx -mno-altivec %s 2>&1 | \
+// RUN: FileCheck %s -check-prefix=CHECK-NALTI-VSX
+
+// RUN: not %clang -target powerpc64le-unknown-unknown -fsyntax-only \
+// RUN: -std=c++11 -msoft-float -maltivec %s 2>&1 | \
+// RUN: FileCheck %s -check-prefix=CHECK-SOFTFLT-ALTI
+
 #ifdef __VSX__
 static_assert(false, "VSX enabled");
 #endif
@@ -112,5 +120,7 @@
 // CHECK-NVSX-MULTI: error: option '-mfloat128' cannot be specified with 
'-mno-vsx'
 // CHECK-NVSX-MULTI: error: option '-mpower9-vector' cannot be specified with 
'-mno-vsx'
 // CHECK-NVSX-MMA: error: option '-mmma' cannot be specified with '-mno-vsx'
+// CHECK-NALTI-VSX: error: option '-mvsx' cannot be specified with 
'-mno-altivec'
+// CHECK-SOFTFLT-ALTI: error: option '-msoft-float' cannot be specified with 
'-maltivec'
 // CHECK-NVSX: Neither enabled
 // CHECK-VSX: VSX enabled
Index: clang/test/CodeGen/PowerPC/attr-target-ppc.c
===
--- clang/test/CodeGen/PowerPC/attr-target-ppc.c
+++ clang/test/CodeGen/PowerPC/attr-target-ppc.c
@@ -1,4 +1,5 @@
 // RUN: not %clang_cc1 -triple powerpc64le-linux-gnu -emit-llvm %s -o -
 
 long __attribute__((target("power8-vector,no-vsx"))) foo (void) { return 0; }  
// expected-error {{option '-mpower8-vector' cannot be specified with 
'-mno-vsx'}}
-
+long __attribute__((target("no-altivec,vsx"))) foo2(void) { return 0; }
// expected-error {{option '-mvsx' cannot be specified with '-mno-altivec'}}
+long __attribute__((target("no-hard-float,altivec"))) foo3(void) { return 0; } 
// expected-error {{option '-msoft-float' cannot be specified with '-maltivec'}}
Index: clang/lib/Basic/Targets/PPC.cpp
===
--- clang/lib/Basic/Targets/PPC.cpp
+++ clang/lib/Basic/Targets/PPC.cpp
@@ -447,6 +447,18 @@
 static bool ppcUserFeaturesCheck(DiagnosticsEngine ,
  const std::vector ) {
 
+  // Cannot allow VSX with no Altivec.
+  if (llvm::is_contained(FeaturesVec, "-hard-float") &&
+  llvm::is_contained(FeaturesVec, "+altivec"))
+Diags.Report(diag::err_opt_not_valid_with_opt) << "-msoft-float"
+   << "-maltivec";
+
+  // Cannot allow soft-float with Altivec.
+  if (llvm::is_contained(FeaturesVec, "+vsx") &&
+  llvm::is_contained(FeaturesVec, "-altivec"))
+Diags.Report(diag::err_opt_not_valid_with_opt) << "-mvsx"
+   << "-mno-altivec";
+
   // vsx was not explicitly turned off.
   if (!llvm::is_contained(FeaturesVec, "-vsx"))
 return true;


Index: clang/test/Driver/ppc-dependent-options.cpp
===
--- clang/test/Driver/ppc-dependent-options.cpp
+++ clang/test/Driver/ppc-dependent-options.cpp
@@ -78,6 +78,14 @@
 // RUN: -mcpu=power10 -std=c++11 -mno-vsx -mpower10-vector %s 2>&1 | \
 // RUN: FileCheck %s -check-prefix=CHECK-NVSX-P10V
 
+// RUN: not %clang -target powerpc64le-unknown-unknown -fsyntax-only \
+// RUN: -std=c++11 -mvsx -mno-altivec %s 2>&1 | \
+// RUN: FileCheck %s -check-prefix=CHECK-NALTI-VSX
+
+// RUN: not %clang -target powerpc64le-unknown-unknown -fsyntax-only \
+// RUN: -std=c++11 -msoft-float -maltivec %s 2>&1 | \
+// RUN: FileCheck %s -check-prefix=CHECK-SOFTFLT-ALTI
+
 #ifdef __VSX__
 static_assert(false, "VSX enabled");
 #endif
@@ -112,5 +120,7 @@
 // 

[PATCH] D125203: [PowerPC] Fix PPCISD::STBRX selection issue on A2

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

In D125203#3502651 , @tingwang wrote:

> In D125203#3502433 , @nemanjai 
> wrote:
>
>> Why not also fix this in the front end so that we allow the builtin on the 
>> A2 CPU as well (since it's supported)?
>
> Oh I missed that. Thank you for pointing out!
>
> Just now updated the patch. However I didn't update the SemaFeatureCheck 
> message to indicate the support on a2, since if people see this error 
> message, it cannot be a2, and a2 does not easily fit into the message of 
> err_ppc_builtin_only_on_arch. Hope this will not create problem.

I wouldn't worry about it. If we have to change this in the future because some 
implementation of the ISA that isn't `pwr` needs to reference it, we can 
change it at that time.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125203

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

[PATCH] D124060: [PowerPC] Enable CR bits support for Power8 and above.

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:3566
 
+Control the CR-bit tracking feature on PowerPC. ``-mcrbits`` (the enablement 
of CR-bit tracking support) is the default for POWER8 and above.
+

```
... is the default for POWER8 and above as well as for all other CPUs when 
optimization is applied (-O2 and above).
```
But please check if it is also applied at -O1 and correct that statement 
accordingly.



Comment at: llvm/test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll:5
 ; CHECK-LABEL: TestULT:
-; CHECK: xscmpudp
+; CHECK: fcmpu
 ; CHECK: blr

Why do we not emit the VSX instructions here any longer? How are crbits related?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124060

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

[PATCH] D124060: [PowerPC] Enable CR bits support for Power8 and above.

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.cpp:519
 .Default(false);
+  Features["crbits"] = llvm::StringSwitch(CPU)
+.Case("ppc64le", true)

shchenz wrote:
> amyk wrote:
> > shchenz wrote:
> > > If we set the `+crbits` by the arch name, do we still need the 
> > > customization (Turn on crbits for O2 and above) in 
> > > `computeFSAdditions()`? 
> > Yeah, that's a good point. I looked into this previously, and it appears 
> > that addition of `-mcrbits` inside `computeFSAdditions()` may still be 
> > necessary. 
> > 
> > In particular, we have test cases that test pre-POWER8 with optimizations 
> > on (or, if no optimization level is provided, then -O2 is assumed the 
> > default). In these cases, much of the code changes if the customization 
> > inside `computeFSAdditions()` is removed because we would no longer be 
> > using crbits pre-P8.
> hmm, OK, then we still have the same issue that this patch fixes for 
> pre-POWER8. I'm ok with leaving it for now as IMO Power7/Power6 should not be 
> major versions for PowerPC.
Up until Power8, the only instructions we had that require `crbits` were 
CR-logicals (which existed since the POWER architecture - i.e. before PowerPC). 
In Power8, we added BCD instructions that also require `crbits`. So that's why 
we turn it on for Power8 and up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124060

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D122983#3465122 , @aaron.ballman 
wrote:

> In D122983#3465099 , @nemanjai 
> wrote:
>
>> @daltenty Can you please run this with the same config as the bot on one of 
>> our AIX machines but be sure to pass `-i` if it's `make` or `-k 0` if it's 
>> `ninja`?
>
> That would be *awesome*! I have to take off for a few hours, but I'm happy to 
> fix all of the issues you spot in one go when I get back in a bit. Thank you!

Actually, I had a build ready on AIX and ran it. Looks like just 2 more:

  test-suite :: 
MultiSource/Benchmarks/MiBench/network-patricia/network-patricia.test
  test-suite :: MultiSource/Benchmarks/nbench/nbench.test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122983

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

@daltenty Can you please run this with the same config as the bot on one of our 
AIX machines but be sure to pass `-i` if it's `make` or `-k 0` if it's `ninja`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122983

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

[PATCH] D124093: [PowerPC] Fixing implicit castings in altivec for -fno-lax-vector-conversions

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Also, please run `clang-format` on the changes.




Comment at: clang/lib/Headers/altivec.h:19051
 #ifdef __LITTLE_ENDIAN__
-  return __builtin_altivec_vstribl_p(__CR6_EQ, (vector signed char)__a);
+  return __builtin_altivec_vstribl_p(__CR6_EQ, (vector char)__a);
 #else

We should never cast anything to an integral vector type that doesn't include 
`signed/unsigned/bool`.



Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-p10vector.c:1955
   // CHECK: sub <1 x i128>
-  // CHECK-NEXT: lshr <1 x i128>
+  // CHECK-NEXT: ashr <1 x i128>
   // CHECK-NEXT: or <1 x i128>

This is not good. We are changing semantics here - turning a logical shift into 
an arithmetic shift.



Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-quadword-noi128.c:10
 // RUN: %clang_cc1 -O2 -target-feature +altivec -target-feature +power8-vector 
\
-// RUN:   -triple powerpc-aix-unknown -emit-llvm %s -o - | FileCheck \
+// RUN:   -triple powerpc-aix-unknown -emit-llvm -fforce-enable-int128 %s -o - 
| FileCheck \
 // RUN:   %s -check-prefix=CHECK-AIX

Why is this addition needed? Seems like something is wrong if we need to force 
enable int128 in order to compile something with `altivec.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124093

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D122983#3463569 , @xbolva00 wrote:

> But your link shows failures in compiler-rt/, not in llvm test-suite

It shows both. But the `compiler-rt` ones have subsequently been fixed while 
the `test-suite` ones are still failing as of 
https://lab.llvm.org/buildbot/#/builders/105/builds/24314.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122983

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

This is still causing failures when building `test-suite`:
https://lab.llvm.org/buildbot/#/builders/105/builds/24292

Why don't we just turn off this warning for the entire `test-suite` since I 
don't expect we want to modify the tests (as some of them have licenses that 
probably don't allow us to modify them)?
We could do something like this:

  diff --git a/CMakeLists.txt b/CMakeLists.txt
  index 8d76a45c7..032281ecf 100644
  --- a/CMakeLists.txt
  +++ b/CMakeLists.txt
  @@ -47,6 +47,7 @@ set(LLVM_CODESIGNING_IDENTITY "" CACHE STRING
 "Sign executables and dylibs with the given identity or skip if empty 
(Darwin Only)")
   
   add_definitions(-DNDEBUG)
  +add_definitions(-Wno-implicit-function-declaration)
   option(TEST_SUITE_SUPPRESS_WARNINGS "Suppress all warnings" ON)
   if(${TEST_SUITE_SUPPRESS_WARNINGS})
 add_definitions(-w)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122983

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

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D67678#3446526 , @rsmith wrote:

> @dexonsmith @arphaman What do we need to do to get this re-landed?

FWIW, on the PPC side, we are working on cleaning up `altivec.h` and plan to 
eventually make `-fno-lax-vector-conversions` the default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67678

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

[PATCH] D121637: [PowerPC] Fix EmitPPCBuiltinExpr to emit arguments once

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai accepted this revision.
nemanjai added a comment.

Please add a test case that would cause a failure prior to this patch due to 
the argument being emitted more than once (i.e. the test case that prompted 
this patch). If that is already added and I just missed it, please add a note 
to the test case along the lines of:

  // The argument expression must not be emitted multiple times.

Other than the minor nits, LGTM.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15627
+Value *Y =
+Builder.CreateAnd(EmitScalarExpr(E->getArg(1)), 
Builder.CreateNot(Op3));
 return Builder.CreateOr(X, Y);

Nit: I understand that we only have one use of `E->getArg(1)`, but might as 
well initialize `Op1` as above just for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121637

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

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D120305#3346880 , @MaskRay wrote:

> While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
> willing to fix issues if I have access to a ppc64 machine (especially 
> compiler-rt ones that I care about),
> I feel uncomfortably if a group just bluntly request "please pull this patch" 
> when apparently (a) there is a better approach (explicitly setting 
> CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer 
> can do
> and (c) there is just some inherent stability problem (in this case, consider 
> not enabling the testing when the target is still unstable) that is causing 
> not only this issue, but various other reports (as I watch sanitizer failures 
> quite closely and ppc64 often tends to be the outlier thing)

Statements like this seem to be at odds with this community's culture (or at 
least the way I understand it).
As long as I have been a member of this community, the guidance for patches 
that break bots is to fix it immediately if the fix is obvious/trivial and if 
it isn't, to pull the patch until a solution can be found. I am not aware of 
any changes to this policy. I would also like to add that this approach serves 
most other members of the community quite well and at least I personally don't 
see much opposition to this. Frankly, the only person I have ever received a 
response that amounts to "I would rather not" when asking them to pull a patch 
that breaks bots is yourself.

So I'll try to respond to the individual statements you have made here.

1. No access to a PowerPC machine - we have given you access to one before and 
are happy to do so again at any time.
2. "bluntly requesting to pull the patch" - This is perhaps the part of your 
statement that I find most surprising. In case you haven't come across this 
, I 
encourage you to have a read through it. If you feel this needs to change, I 
encourage you to bring it up for discussion with the wider community. Of 
particular interest is this sentence: `We strongly encourage “revert to green” 
as opposed to “fixing forward”. We encourage reverting first, investigating 
offline, and then reapplying the fixed patch - possibly after another round of 
review if warranted.`
3. "there is a better approach" - I don't think I have to spend a lot of time 
explaining how subjective this statement is.
4. "there is something that a bot maintainer can do" - I can't quite decipher 
whether this is a disingenuous statement pretending that this is a situation 
where the bot maintainer (effectively myself in this case) isn't willing to 
help. Or perhaps it is a statement you made in the heat of the moment when I 
asked you to pull the patch and you missed the part where I offered help to 
resolve the issues when normal business resumes. I will give you the benefit of 
assuming the latter as I truly don't believe you have ill intentions here. I 
would just like to add that, as you surely realize, bot maintainers are not 
sitting around waiting for someone to break their bot so they can jump in 
immediately and work on resolving the issue. As contributors to this project, 
it is our responsibility to keep the tip of trunk green and to work with bot 
maintainers on their own time (within reason) to resolve issues we cause on 
their bots.
5. The rather surprising and discouraging statement you made under `(c)` - 
while I realize that PowerPC may not be a target on which you develop, it is 
really not fair to make a blanket statement that PowerPC is not stable wrt. 
sanitizers. If you feel there is a stability issue with a specific bot or a 
specific target, I encourage you to collect data about false failures and bring 
it up with us - we would be happy to look into it as I am sure would any other 
target/bot maintainer. Statements like this sow the seeds of resentment towards 
specific targets - you are effectively saying that PowerPC is an unstable 
target (at least wrt. to sanitizers) but we insist on burdening the community 
with our unstable target by running sanitizer tests in our bots. I am afraid 
that unlike number 4. above, I don't find any ambiguity in your statement here. 
Your statement seems to be clearly and unambiguously hostile towards PowerPC 
and as such, I find it at odds with the spirit of this community. Regardless of 
all of that, I would once again like to reiterate that I would be very happy to 
look into false failures you are encountering with sanitizers on PowerPC.

Fangrui, I would really like you to understand that I very much value your 
contribution to various LLVM projects. You are an exceptional developer and 
seem to be laser focused on advancing the state of LLVM and its subprojects. 
This is not at all lost on me. However, you need to understand that this 
community has all kinds of participants, many of which also care about older or 

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D120305#3346144 , @MaskRay wrote:

> In D120305#3345978 , @MaskRay wrote:
>
>> In D120305#3345810 , @RKSimon 
>> wrote:
>>
>>> @MaskRay The ppc buildbots have been red since these patches - please can 
>>> you take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454
>>
>> Seems that ppc64 doesn't support test/sanitizer_common test/crt -fpie. I'll 
>> just disable them: https://github.com/llvm/llvm-project/issues/54084
>
> Actually I don't know how to disable the tests: 
> https://lab.llvm.org/buildbot/#/builders/57/builds/15497 still failed.
> Hope someone from #powerpc  can 
> disable them.

This does not appear to be a matter of simply marking some tests as 
UNSUPPORTED. Since this landed, there have been many builds with different 
sanitizer failures and different numbers of sanitizer failures. Please pull 
this patch to bring the bots back to green and we can work with you next week 
on fixing what needs to be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120305

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

[PATCH] D118753: [PowerPC] Fix __builtin_pdepd and __builtin_pextd to be 64-bit and P10 only.

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118753

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

[PATCH] D118350: [Clang][Sema][AIX][PowerPC] Emit byval alignment warning only when struct member is passed to a function

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Would it make sense (and would it be possible) to check the linkage of the 
callee? Presumably calling something like `static void localfunc(int *)` with 
an over-aligned member shouldn't be a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D116085#3272200 , @nemanjai wrote:

> This is causing buildbot failures with `-Werror` (i.e. 
> https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). 
> Please fix or revert. The failure is:
>
>   
> .../llvm-project/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:204:38:
>  error: moving a temporary object prevents copy elision 
> [-Werror,-Wpessimizing-move]
>   CompletedBlocks.emplace_back(std::move(Stack.pop_back_val()), 
> NoLint);

Sorry, I didn't realize you already reverted as there was no mention in the 
revert of the original commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

This is causing buildbot failures with `-Werror` (i.e. 
https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). 
Please fix or revert. The failure is:

  
.../llvm-project/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:204:38:
 error: moving a temporary object prevents copy elision 
[-Werror,-Wpessimizing-move]
  CompletedBlocks.emplace_back(std::move(Stack.pop_back_val()), NoLint);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D118211: Add missing namespace to PPCLinux.cpp

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118211

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

[PATCH] D118110: [CMake] [Clang] Add CMake build option to specify long double format on PowerPC

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/CMakeLists.txt:240
 
+set(ENABLE_PPC_IEEELONGDOUBLE OFF CACHE BOOL
+"Enable IEEE binary128 as default long double format on PowerPC.")

Do we need any error checking here? What happens if someone erroneously sets 
this on an AIX build (or big endian Linux, FreeBSD, etc.)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118110

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

[PATCH] D118110: [CMake] [Clang] Add CMake build option to specify long double format on PowerPC

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

It is probably not worth the effort since there won't be that many test cases 
that test the front end's IR generation for `long double`, but there should be 
a way to set up lit to know the default through its configuration files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118110

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

[PATCH] D112906: [PowerPC] Emit warning for ieeelongdouble on older GNU toolchain

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/lib/Driver/ToolChains/PPCLinux.cpp:50
+  if (!SupportIEEEFloat128(D, Triple, Args)) {
+if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ)) {
+  StringRef ABIName = A->getValue();

Seems that this will be less frequent than every compiler invocation, so we 
should probably check this first and early exit if the option isn't specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112906

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

[PATCH] D117181: [PowerPC] Use IEEE long double in proper toolchain

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D117181#3261396 , @jsji wrote:

> So, the proposal is we change the default on Linux,  so we will not change 
> the default based on the gcc version, it is based on OS and clang version 
> only.
> but will emit warning if we detect that the GCC toolchain is too old.
>
>   Can we just update bool IEEELongDouble = false;  to bool IEEELongDouble = 
> T.isOSLinux(); in clang/lib/Driver/ToolChains/Clang.cpp?

Won't that end up producing a warning on ALL code built on any Linux distro 
with a GCC toolchain older than 12.1? That would be terrible.
Ultimately, what we care about is checking for the default for the distro 
toolchain. I am not sure what a good proxy for that is, but I don't think it is 
"all Linux distros".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117181

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

[PATCH] D117181: [PowerPC] Use IEEE long double in proper toolchain

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

I don't think this is the right approach. Personally, I would find it 
surprising if simply having GCC 12.1 on the system changes my `long double` 
default. Imagine if I simply install and use such a toolchain on a machine for 
whatever reason. If the default for such a toolchain is not IEEE, then the 
clang default diverges from the GCC toolchain default.

Would there be a way to query the GCC toolchain for what the default is for 
`long double`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117181

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

[PATCH] D112073: [PowerPC] Emit warning when SP is clobbered by asm

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112073

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

[PATCH] D106120: [PowerPC] Implement vector bool/pixel initialization under -faltivec-src-compat=xl

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D106120#3197209 , @q66 wrote:

> Well, I'm more concerned about the actual semantics - as I see it, when using 
> generic vectors, they should be unaffected by the comparison behavior of 
> AltiVec vectors, which is not the case when you set the mode to `xl`. If the 
> semantics are affected, it means creating a massive pain for libraries that 
> use generic vectors without caring what platform they are in (since they will 
> get a vector as a result of comparison on x86_64, but a bool on PowerPC). And 
> since the behavior can be switched with a compiler flag, they don't even have 
> a reasonable way to make this conditional in the code.

Only users that are looking to match the XL behaviour will ever use the `xl` 
mode. I realize that the warning incorrectly states that the default behaviour 
will be switched to XL in the future but this is wrong (and we will correct 
it). Unfortunately with the XL compiler, the behaviour is to produce a 
`bool/int` when comparing two vectors regardless of whether they are Altivec 
vectors or generic ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106120

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

[PATCH] D116015: [PowerPC] Add generic fnmsub intrinsic

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Converting more generic code to target-specific intrinsics is sometimes 
necessary to ensure the generic IR doesn't get transformed in a way that is 
disadvantageous. I believe that the description of this review claims that to 
be the case for these negated FMA's. The obvious disadvantage of producing 
target-specific intrinsics is that the optimizer knows nothing about them so no 
advantageous transformations can happen either (i.e. hiding the semantics from 
the optimizer is sometimes a good thing and sometimes a bad thing).

The description of this patch includes no test case that shows the optimizer 
performing an undesirable transformation. So the motivation for making the 
front end produce more opaque code is not at all clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116015

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

[PATCH] D116016: [Clang] [PowerPC] Emit module flag for current float abi

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

We should not be emitting the attribute in modules that do not have any use of 
`long double`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116016

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

[PATCH] D106120: [PowerPC] Implement vector bool/pixel initialization under -faltivec-src-compat=xl

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D106120#3196010 , @q66 wrote:

> On x86_64, the program predictably prints `Dv4_i`.

The semantics here are not new, but the warning is emitted for generic vector 
types as a result of https://reviews.llvm.org/D103615. This is perhaps an 
oversight that should change (i.e. only emit the warning from 
`CheckVectorCompareOperands()` if the operands are `vector bool/vector pixel`).
@stefanp What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106120

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

[PATCH] D114497: [PowerPC] Drop stdlib paths in freestanding tests

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Seems fine to me but maybe give @MaskRay a couple of days to see if this 
adequately addresses his comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114497

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

[PATCH] D114540: Big-endian version of vpermxor

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM.




Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2506
+  def : Pat<(v16i8 (int_ppc_altivec_crypto_vpermxor_be v16i8:$a,
+v16i8:$b, v16i8:$c)),
+(v16i8 (VPERMXOR $a, $b, $c))>;

Nit: the `v16i8` should be aligned with the `v16i8` above.



Comment at: llvm/test/CodeGen/PowerPC/crypto_bifs_be.ll:8
+; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefixes=CHECK-BE-P8
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
+; RUN:   -mcpu=pwr9 < %s | FileCheck %s --check-prefixes=CHECK-P9

There isn't really any difference in how we handle this on P9 so this run line 
is probably superfluous. Feel free to get rid of it when pushing this upstream.


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

https://reviews.llvm.org/D114540

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

[PATCH] D114540: Big-endian version of vpermxor

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Please also add a test for this builtin to the front end test 
`clang/test/CodeGen/builtins-ppc-crypto.c`




Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2499
  (COPY_TO_REGCLASS $c, VSRC>;
+  def : Pat<(v16i8 (int_ppc_altivec_crypto_vpermxor_be v16i8:$a,
+v16i8:$b, v16i8:$c)),

Although it would be redundant, it is not invalid to use this builtin/intrinsic 
in BE compilations. As such, please move this to a block that has only the
`[HasVSX, HasP8Altivec]` predicates as the desired code is the same for LE/BE.



Comment at: llvm/test/CodeGen/PowerPC/crypto_bifs_be.ll:9
+entry:
+  %a = alloca <16 x i8>, align 16
+  %b = alloca <16 x i8>, align 16

Please clean up this test case a bit:
- Run `opt --passes="default"` on it to clean up the unnecessary code
- Remove the attributes
- The 755 permissions on the test case are unusual
- Produce the CHECK's automatically using 
`$LLVM_SRC/utils/update_llc_test_checks.py`
- Add a BE invocation (either the P8 or the P9 can just use the 
`powerpc64-unknown-linux-gnu` triple)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114540

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

[PATCH] D113642: [PowerPC] Provide XL-compatible vec_round implementation

[Nemanja Ivanovic via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb7bf937bbee3: [PowerPC] Provide XL-compatible vec_round 
implementation (authored by nemanjai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113642

Files:
  clang/lib/Headers/altivec.h
  clang/test/CodeGen/builtins-ppc-vsx.c
  clang/test/CodeGen/builtins-ppc-xlcompat.c
  llvm/lib/Target/PowerPC/PPCISelLowering.cpp
  llvm/test/CodeGen/PowerPC/read-set-flm.ll

Index: llvm/test/CodeGen/PowerPC/read-set-flm.ll
===
--- llvm/test/CodeGen/PowerPC/read-set-flm.ll
+++ llvm/test/CodeGen/PowerPC/read-set-flm.ll
@@ -11,7 +11,6 @@
 ; CHECK-NEXT:xsdivdp 1, 1, 2
 ; CHECK-NEXT:xsadddp 1, 1, 3
 ; CHECK-NEXT:xsadddp 0, 1, 0
-; CHECK-NEXT:mffs 1
 ; CHECK-NEXT:mtfsf 255, 4
 ; CHECK-NEXT:xsdivdp 1, 3, 4
 ; CHECK-NEXT:xsadddp 1, 1, 2
@@ -47,7 +46,6 @@
 ; CHECK-NEXT:xsdivdp 1, 1, 2
 ; CHECK-NEXT:xsadddp 1, 1, 3
 ; CHECK-NEXT:xsadddp 0, 1, 0
-; CHECK-NEXT:mffs 1
 ; CHECK-NEXT:mtfsf 255, 4
 ; CHECK-NEXT:xsdivdp 1, 3, 4
 ; CHECK-NEXT:xsadddp 1, 1, 2
@@ -96,7 +94,6 @@
 ; CHECK-NEXT:nop
 ; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:stfd 0, 0(30)
-; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:mtfsf 255, 31
 ; CHECK-NEXT:addi 1, 1, 64
 ; CHECK-NEXT:ld 0, 16(1)
@@ -134,7 +131,6 @@
 ; CHECK-NEXT:nop
 ; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:stfd 0, 0(30)
-; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:mtfsf 255, 31
 ; CHECK-NEXT:addi 1, 1, 64
 ; CHECK-NEXT:ld 0, 16(1)
Index: llvm/lib/Target/PowerPC/PPCISelLowering.cpp
===
--- llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -12116,6 +12116,7 @@
   MachineFunction::iterator It = ++BB->getIterator();
 
   MachineFunction *F = BB->getParent();
+  MachineRegisterInfo  = F->getRegInfo();
 
   if (MI.getOpcode() == PPC::SELECT_CC_I4 ||
   MI.getOpcode() == PPC::SELECT_CC_I8 || MI.getOpcode() == PPC::SELECT_I4 ||
@@ -12721,7 +12722,10 @@
 Register OldFPSCRReg = MI.getOperand(0).getReg();
 
 // Save FPSCR value.
-BuildMI(*BB, MI, dl, TII->get(PPC::MFFS), OldFPSCRReg);
+if (MRI.use_empty(OldFPSCRReg))
+  BuildMI(*BB, MI, dl, TII->get(TargetOpcode::IMPLICIT_DEF), OldFPSCRReg);
+else
+  BuildMI(*BB, MI, dl, TII->get(PPC::MFFS), OldFPSCRReg);
 
 // The floating point rounding mode is in the bits 62:63 of FPCSR, and has
 // the following settings:
@@ -12854,7 +12858,10 @@
 
 // Result of setflm is previous FPSCR content, so we need to save it first.
 Register OldFPSCRReg = MI.getOperand(0).getReg();
-BuildMI(*BB, MI, Dl, TII->get(PPC::MFFS), OldFPSCRReg);
+if (MRI.use_empty(OldFPSCRReg))
+  BuildMI(*BB, MI, Dl, TII->get(TargetOpcode::IMPLICIT_DEF), OldFPSCRReg);
+else
+  BuildMI(*BB, MI, Dl, TII->get(PPC::MFFS), OldFPSCRReg);
 
 // Put bits in 32:63 to FPSCR.
 Register NewFPSCRReg = MI.getOperand(1).getReg();
Index: clang/test/CodeGen/builtins-ppc-xlcompat.c
===
--- clang/test/CodeGen/builtins-ppc-xlcompat.c
+++ clang/test/CodeGen/builtins-ppc-xlcompat.c
@@ -5,11 +5,16 @@
 // RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx \
 // RUN:   -triple powerpc64le-unknown-linux-gnu -emit-llvm %s -o - \
 // RUN:   -D__XL_COMPAT_ALTIVEC__ -target-cpu pwr8 | FileCheck %s
+// RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx \
+// RUN:   -triple powerpc64le-unknown-linux-gnu -emit-llvm %s -o - \
+// RUN:   -U__XL_COMPAT_ALTIVEC__ -target-cpu pwr8 | FileCheck \
+// RUN:   --check-prefix=NOCOMPAT %s
 #include 
 vector double vd = { 3.4e22, 1.8e-3 };
 vector signed long long vsll = { -12345678999ll, 12345678999 };
 vector unsigned long long vull = { 11547229456923630743llu, 18014402265226391llu };
 vector float res_vf;
+vector double res_vd;
 vector signed int res_vsi;
 vector unsigned int res_vui;
 
@@ -38,4 +43,11 @@
 // CHECK: [[TMP8:%.*]] = load <2 x double>, <2 x double>* @vd, align 16
 // CHECK-NEXT:fmul <2 x double> [[TMP8]], 
 // CHECK: call <4 x i32> @llvm.ppc.vsx.xvcvdpuxws(<2 x double>
+
+  res_vd = vec_round(vd);
+// CHECK: call double @llvm.ppc.readflm()
+// CHECK: call double @llvm.ppc.setrnd(i32 0)
+// CHECK: call <2 x double> @llvm.rint.v2f64(<2 x double>
+// CHECK: call double @llvm.ppc.setflm(double
+// NOCOMPAT:  call <2 x double> @llvm.round.v2f64(<2 x double>
 }
Index: clang/test/CodeGen/builtins-ppc-vsx.c
===
--- clang/test/CodeGen/builtins-ppc-vsx.c
+++ clang/test/CodeGen/builtins-ppc-vsx.c
@@ -409,10 +409,6 @@
 // CHECK: call <4 x float> 

[PATCH] D114088: [PowerPC] Add BCD add/sub/cmp builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc933c2eb3346: [PowerPC] Add BCD add/sub/cmp builtins 
(authored by nemanjai).

Changed prior to commit:
  https://reviews.llvm.org/D114088?vs=387933=389238#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114088

Files:
  llvm/include/llvm/IR/IntrinsicsPowerPC.td
  llvm/lib/Target/PowerPC/P10InstrResources.td
  llvm/lib/Target/PowerPC/P9InstrResources.td
  llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
  llvm/lib/Target/PowerPC/PPCInstrAltivec.td
  llvm/test/CodeGen/PowerPC/bcd-intrinsics.ll

Index: llvm/test/CodeGen/PowerPC/bcd-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/bcd-intrinsics.ll
@@ -0,0 +1,212 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   -mcpu=pwr10 -ppc-asm-full-reg-names \
+; RUN:   -ppc-vsr-nums-as-vr < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
+; RUN:   -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -ppc-vsr-nums-as-vr < %s | FileCheck %s --check-prefix=CHECK-P9
+
+define dso_local i64 @test_invalid(<16 x i8> %a) local_unnamed_addr #0 {
+; CHECK-LABEL: test_invalid:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v2, 0
+; CHECK-NEXT:setbc r3, 4*cr6+un
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_invalid:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v2, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 28, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call i32 @llvm.ppc.bcdsub.p(i32 6, <16 x i8> %a, <16 x i8> %a) #2
+  %conv.i = sext i32 %0 to i64
+  ret i64 %conv.i
+}
+
+define dso_local <16 x i8> @test_add(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_add:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdadd. v2, v2, v3, 1
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_add:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdadd. v2, v2, v3, 1
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call <16 x i8> @llvm.ppc.bcdadd(<16 x i8> %a, <16 x i8> %b, i32 1)
+  ret <16 x i8> %0
+}
+
+define dso_local i64 @test_add_ofl(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_add_ofl:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdadd. v2, v2, v3, 0
+; CHECK-NEXT:setbc r3, 4*cr6+un
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_add_ofl:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdadd. v2, v2, v3, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 28, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call i32 @llvm.ppc.bcdadd.p(i32 6, <16 x i8> %a, <16 x i8> %b) #2
+  %conv.i = sext i32 %0 to i64
+  ret i64 %conv.i
+}
+
+define dso_local <16 x i8> @test_sub(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_sub:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_sub:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call <16 x i8> @llvm.ppc.bcdsub(<16 x i8> %a, <16 x i8> %b, i32 0)
+  ret <16 x i8> %0
+}
+
+define dso_local i64 @test_sub_ofl(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_sub_ofl:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-NEXT:setbc r3, 4*cr6+un
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_sub_ofl:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 28, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call i32 @llvm.ppc.bcdsub.p(i32 6, <16 x i8> %a, <16 x i8> %b) #2
+  %conv.i = sext i32 %0 to i64
+  ret i64 %conv.i
+}
+
+define dso_local i64 @test_cmplt(<16 x i8> %a, <16 x i8> %b) local_unnamed_addr #0 {
+; CHECK-LABEL: test_cmplt:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-NEXT:setbc r3, 4*cr6+lt
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_cmplt:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 25, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call i32 @llvm.ppc.bcdsub.p(i32 2, <16 x i8> %a, <16 x i8> %b) #2
+  %conv.i = sext i32 %0 to i64
+  ret i64 %conv.i
+}
+
+define 

[PATCH] D114088: [PowerPC] Add BCD add/sub/cmp builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: clang/lib/Headers/altivec.h:19050
+}
+
+static __inline__ long __bcdcmpeq(vector unsigned char __a,

NeHuang wrote:
> Do we need to add a case for "__CR6_SO_REV"? It is defined in line 25 but not 
> used.
I added it for completeness, but there is currently no need to use it for any 
builtin.



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5096
+SubReg = PPC::sub_un;
+ShiftVal = 0;
+break;

NeHuang wrote:
> can we remove this as default is 0?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114088

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

[PATCH] D114088: [PowerPC] Add BCD add/sub/cmp builtins

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai created this revision.
nemanjai added reviewers: bmahjour, PowerPC.
Herald added subscribers: shchenz, kbarton, hiraditya.
nemanjai requested review of this revision.
Herald added projects: clang, LLVM.

Support for builtins that use `bcdadd./bcdsub.` to add/subtract Binary Coded 
Decimal values as well as to determine validity and compare BCD values.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114088

Files:
  clang/include/clang/Basic/BuiltinsPPC.def
  clang/lib/Headers/altivec.h
  clang/test/CodeGen/builtins-ppc-p8vector.c
  llvm/include/llvm/IR/IntrinsicsPowerPC.td
  llvm/lib/Target/PowerPC/P10InstrResources.td
  llvm/lib/Target/PowerPC/P9InstrResources.td
  llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
  llvm/lib/Target/PowerPC/PPCInstrAltivec.td
  llvm/test/CodeGen/PowerPC/bcd-intrinsics.ll

Index: llvm/test/CodeGen/PowerPC/bcd-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/bcd-intrinsics.ll
@@ -0,0 +1,212 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   -mcpu=pwr10 -ppc-asm-full-reg-names \
+; RUN:   -ppc-vsr-nums-as-vr < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
+; RUN:   -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -ppc-vsr-nums-as-vr < %s | FileCheck %s --check-prefix=CHECK-P9
+
+define dso_local i64 @test_invalid(<16 x i8> %a) local_unnamed_addr #0 {
+; CHECK-LABEL: test_invalid:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v2, 0
+; CHECK-NEXT:setbc r3, 4*cr6+un
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_invalid:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v2, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 28, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call i32 @llvm.ppc.bcdsub.p(i32 6, <16 x i8> %a, <16 x i8> %a) #2
+  %conv.i = sext i32 %0 to i64
+  ret i64 %conv.i
+}
+
+define dso_local <16 x i8> @test_add(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_add:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdadd. v2, v2, v3, 1
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_add:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdadd. v2, v2, v3, 1
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call <16 x i8> @llvm.ppc.bcdadd(<16 x i8> %a, <16 x i8> %b, i32 1)
+  ret <16 x i8> %0
+}
+
+define dso_local i64 @test_add_ofl(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_add_ofl:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdadd. v2, v2, v3, 0
+; CHECK-NEXT:setbc r3, 4*cr6+un
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_add_ofl:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdadd. v2, v2, v3, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 28, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call i32 @llvm.ppc.bcdadd.p(i32 6, <16 x i8> %a, <16 x i8> %b) #2
+  %conv.i = sext i32 %0 to i64
+  ret i64 %conv.i
+}
+
+define dso_local <16 x i8> @test_sub(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_sub:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_sub:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call <16 x i8> @llvm.ppc.bcdsub(<16 x i8> %a, <16 x i8> %b, i32 0)
+  ret <16 x i8> %0
+}
+
+define dso_local i64 @test_sub_ofl(<16 x i8> %a, <16 x i8> %b, i64 %ps) local_unnamed_addr #0 {
+; CHECK-LABEL: test_sub_ofl:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-NEXT:setbc r3, 4*cr6+un
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_sub_ofl:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 28, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; CHECK-P9-NEXT:blr
+entry:
+  %0 = tail call i32 @llvm.ppc.bcdsub.p(i32 6, <16 x i8> %a, <16 x i8> %b) #2
+  %conv.i = sext i32 %0 to i64
+  ret i64 %conv.i
+}
+
+define dso_local i64 @test_cmplt(<16 x i8> %a, <16 x i8> %b) local_unnamed_addr #0 {
+; CHECK-LABEL: test_cmplt:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-NEXT:setbc r3, 4*cr6+lt
+; CHECK-NEXT:extsw r3, r3
+; CHECK-NEXT:blr
+;
+; CHECK-P9-LABEL: test_cmplt:
+; CHECK-P9:   # %bb.0: # %entry
+; CHECK-P9-NEXT:bcdsub. v2, v2, v3, 0
+; CHECK-P9-NEXT:mfocrf r3, 2
+; CHECK-P9-NEXT:rlwinm r3, r3, 25, 31, 31
+; CHECK-P9-NEXT:extsw r3, r3
+; 

[PATCH] D113306: [PowerPC] Allow MMA built-ins to accept non-void pointers and arrays

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM. Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113306

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

[PATCH] D109751: [Clang] Support conversion between PPC double-double and IEEE float128

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

Please provide a description for this patch which includes justification for 
why we want to allow conversion between the two types.
I am of the impression that allowing the two types to coexist in completely 
disjoint code should be fine, but I really don't see a compelling reason to 
allow conversions between the two types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109751

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

[PATCH] D111434: [PowerPC] PPC backend optimization on conditional trap intrustions

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM. There are some very minor nits that can be addressed on the commit.




Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1018
+unsigned Opcode2 = LiMI2->getOpcode();
+bool isOperand2Immeidate = MI.getOperand(2).isImm();
+// We can only do the optimization for the "reg + reg" form.

amyk wrote:
> 
Nit: naming convention - variables start with uppercase letters.



Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1019
+bool isOperand2Immeidate = MI.getOperand(2).isImm();
+// We can only do the optimization for the "reg + reg" form.
+if (!(LiMI1 && (Opcode1 == PPC::LI || Opcode1 == PPC::LI8)))

I don't understand this comment. You say we can only do the optimization for 
the "reg + reg" form but the second condition is actually for the "reg + imm" 
form.



Comment at: llvm/test/CodeGen/PowerPC/mi-peepholes-trap-opt.mir:5
+---
+name:conditional_trap_opt_reg_implicit_def
+alignment:   16

Please add a couple more tests:
1. Test case where we delete the instruction because it won't trap
2. Test case(s) where we do some combination of comparisons (NE(24): `<>`, 
LE(20): `<=`, etc.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111434

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

[PATCH] D113642: [PowerPC] Provide XL-compatible vec_round implementation

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai created this revision.
nemanjai added reviewers: PowerPC, rzurob, qiongsiwu.
Herald added subscribers: shchenz, kbarton, hiraditya.
nemanjai requested review of this revision.
Herald added projects: clang, LLVM.

The XL implementation of `vec_round` for `vector double` uses 
"round-to-nearest, ties to even" just as the `vector float` version does. 
However clang and gcc use "round-to-nearest-away" for `vector double` and 
"round-to-nearest, ties to even" for `vector float`.

The XL behaviour is implemented under the `__XL_COMPAT_ALTIVEC__` macro 
similarly to other instances of incompatibility.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113642

Files:
  clang/lib/Headers/altivec.h
  clang/test/CodeGen/builtins-ppc-vsx.c
  clang/test/CodeGen/builtins-ppc-xlcompat.c
  llvm/lib/Target/PowerPC/PPCISelLowering.cpp
  llvm/test/CodeGen/PowerPC/read-set-flm.ll

Index: llvm/test/CodeGen/PowerPC/read-set-flm.ll
===
--- llvm/test/CodeGen/PowerPC/read-set-flm.ll
+++ llvm/test/CodeGen/PowerPC/read-set-flm.ll
@@ -11,7 +11,6 @@
 ; CHECK-NEXT:xsdivdp 1, 1, 2
 ; CHECK-NEXT:xsadddp 1, 1, 3
 ; CHECK-NEXT:xsadddp 0, 1, 0
-; CHECK-NEXT:mffs 1
 ; CHECK-NEXT:mtfsf 255, 4
 ; CHECK-NEXT:xsdivdp 1, 3, 4
 ; CHECK-NEXT:xsadddp 1, 1, 2
@@ -47,7 +46,6 @@
 ; CHECK-NEXT:xsdivdp 1, 1, 2
 ; CHECK-NEXT:xsadddp 1, 1, 3
 ; CHECK-NEXT:xsadddp 0, 1, 0
-; CHECK-NEXT:mffs 1
 ; CHECK-NEXT:mtfsf 255, 4
 ; CHECK-NEXT:xsdivdp 1, 3, 4
 ; CHECK-NEXT:xsadddp 1, 1, 2
@@ -96,7 +94,6 @@
 ; CHECK-NEXT:nop
 ; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:stfd 0, 0(30)
-; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:mtfsf 255, 31
 ; CHECK-NEXT:addi 1, 1, 64
 ; CHECK-NEXT:ld 0, 16(1)
@@ -134,7 +131,6 @@
 ; CHECK-NEXT:nop
 ; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:stfd 0, 0(30)
-; CHECK-NEXT:mffs 0
 ; CHECK-NEXT:mtfsf 255, 31
 ; CHECK-NEXT:addi 1, 1, 64
 ; CHECK-NEXT:ld 0, 16(1)
Index: llvm/lib/Target/PowerPC/PPCISelLowering.cpp
===
--- llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -12122,6 +12122,7 @@
   MachineFunction::iterator It = ++BB->getIterator();
 
   MachineFunction *F = BB->getParent();
+  MachineRegisterInfo  = F->getRegInfo();
 
   if (MI.getOpcode() == PPC::SELECT_CC_I4 ||
   MI.getOpcode() == PPC::SELECT_CC_I8 || MI.getOpcode() == PPC::SELECT_I4 ||
@@ -12727,7 +12728,10 @@
 Register OldFPSCRReg = MI.getOperand(0).getReg();
 
 // Save FPSCR value.
-BuildMI(*BB, MI, dl, TII->get(PPC::MFFS), OldFPSCRReg);
+if (MRI.use_empty(OldFPSCRReg))
+  BuildMI(*BB, MI, dl, TII->get(TargetOpcode::IMPLICIT_DEF), OldFPSCRReg);
+else
+  BuildMI(*BB, MI, dl, TII->get(PPC::MFFS), OldFPSCRReg);
 
 // The floating point rounding mode is in the bits 62:63 of FPCSR, and has
 // the following settings:
@@ -12860,7 +12864,10 @@
 
 // Result of setflm is previous FPSCR content, so we need to save it first.
 Register OldFPSCRReg = MI.getOperand(0).getReg();
-BuildMI(*BB, MI, Dl, TII->get(PPC::MFFS), OldFPSCRReg);
+if (MRI.use_empty(OldFPSCRReg))
+  BuildMI(*BB, MI, Dl, TII->get(TargetOpcode::IMPLICIT_DEF), OldFPSCRReg);
+else
+  BuildMI(*BB, MI, Dl, TII->get(PPC::MFFS), OldFPSCRReg);
 
 // Put bits in 32:63 to FPSCR.
 Register NewFPSCRReg = MI.getOperand(1).getReg();
Index: clang/test/CodeGen/builtins-ppc-xlcompat.c
===
--- clang/test/CodeGen/builtins-ppc-xlcompat.c
+++ clang/test/CodeGen/builtins-ppc-xlcompat.c
@@ -5,11 +5,16 @@
 // RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx \
 // RUN:   -triple powerpc64le-unknown-linux-gnu -emit-llvm %s -o - \
 // RUN:   -D__XL_COMPAT_ALTIVEC__ -target-cpu pwr8 | FileCheck %s
+// RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx \
+// RUN:   -triple powerpc64le-unknown-linux-gnu -emit-llvm %s -o - \
+// RUN:   -U__XL_COMPAT_ALTIVEC__ -target-cpu pwr8 | FileCheck \
+// RUN:   --check-prefix=NOCOMPAT %s
 #include 
 vector double vd = { 3.4e22, 1.8e-3 };
 vector signed long long vsll = { -12345678999ll, 12345678999 };
 vector unsigned long long vull = { 11547229456923630743llu, 18014402265226391llu };
 vector float res_vf;
+vector double res_vd;
 vector signed int res_vsi;
 vector unsigned int res_vui;
 
@@ -38,4 +43,11 @@
 // CHECK: [[TMP8:%.*]] = load <2 x double>, <2 x double>* @vd, align 16
 // CHECK-NEXT:fmul <2 x double> [[TMP8]], 
 // CHECK: call <4 x i32> @llvm.ppc.vsx.xvcvdpuxws(<2 x double>
+
+  res_vd = vec_round(vd);
+// CHECK: call double @llvm.ppc.readflm()
+// CHECK: call double @llvm.ppc.setrnd(i32 0)
+// CHECK: call <2 x double> @llvm.rint.v2f64(<2 x double>
+// CHECK: call double @llvm.ppc.setflm(double
+// NOCOMPAT:  

[PATCH] D112285: [PowerPC] PPC backend optimization to lower int_ppc_tdw/int_ppc_tw intrinsics to TDI/TWI machine instructions

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

LGTM other than a number of stylistic changes. Feel free to address those on 
the commit. You also might want to give @amyk a bit of time to ensure her 
comments were adequately addressed.




Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5017
+Opcode = IntrinsicID == Intrinsic::ppc_tdw ? PPC::TD : PPC::TW;
+  }
+  // We will emit PPC::TDI or PPC::TWI if the 2nd and 3rd operands are reg 
+

Nit: no braces for a single statement.



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5020
+  // imm or imm + reg.
+  else {
+Opcode = IntrinsicID == Intrinsic::ppc_tdw ? PPC::TDI : PPC::TWI;

Nit: keep the `else` on the line immediately following the end of the `if` 
block so it is visually easy to match them up (the comments for `else/else if` 
go into the block). Also, rather than the structure:
```
if (something) {
} else {
  if (something else)
...
  else
...
}
```
Opt for a more flat structure of
```
if (something)
else if (something else)
else
```



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5021
+  else {
+Opcode = IntrinsicID == Intrinsic::ppc_tdw ? PPC::TDI : PPC::TWI;
+// The 2nd and 3rd operands are reg + imm.

If you initialize `Opcode` this way at the declaration, you won't need this 
here and can flatten this as per my above comment.



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5025
+  Ops[2] = getI32Imm(int(SImmOperand3) & 0x, dl);
+}
+// The 2nd and 3rd operands are imm + reg.

Nit: no braces with a single statement.



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5037
+TO = (TO & 0x1) ? TO + 1 : TO - 1;
+  // We swap the fourth and fifthy bit of TO if they are not same.
+  if ((TO & 0x8) != ((TO & 0x10) >> 1))

s/fifthy/fifth


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112285

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

[PATCH] D106409: [PowerPC] Truncate exponent parameter for vec_cts,vec_ctf

[Nemanja Ivanovic via Phabricator via cfe-commits]

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

I believe you are planning an update for this patch. Requesting changes to take 
it off the queue until you have uploaded the updated version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106409

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

[PATCH] D112285: [PowerPC] PPC backend optimization to lower int_ppc_tdw/int_ppc_tw intrinsics to TDI/TWI machine instructions

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai requested changes to this revision.
nemanjai added inline comments.
This revision now requires changes to proceed.



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5001
+  case ISD::INTRINSIC_VOID: {
+if (N->getConstantOperandVal(1) == Intrinsic::ppc_tdw ||
+N->getConstantOperandVal(1) == Intrinsic::ppc_tw) {

amyk wrote:
> Might be a good idea to save `N->getConstantOperandVal(1)` since it is being 
> accessed quite a few times here.
+1



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5003
+N->getConstantOperandVal(1) == Intrinsic::ppc_tw) {
+  unsigned Opcode = 0;
+  int16_t SImmOperand2;

amyk wrote:
> I see we emit TDI/TWI in 2/3 cases, so I was wondering if it make sense pull 
> out setting the opcode in the second and third case to have the default 
> opcode be:
> ```
> Opcode = N->getConstantOperandVal(1) == Intrinsic::ppc_tdw ? PPC::TDI
>: PPC::TWI;
> ```
> And then we just set the opcode to TD/TW in the first case?
+1
Same thing with the `Ops` vector. Pre-populate it and then only change the 
operand that needs to be changed.



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5012
+  isIntS16Immediate(N->getOperand(3), SImmOperand3);
+  // Will emit TD/TW if 2nd and 3rd operands are reg + reg or imm + imm
+  if (isOperand2IntS16Immediate == isOperand3IntS16Immediate) {

Nit: complete sentences please. Here and in other comments.

Also, please add a comment stating that the `imm + imm` form will be optimized 
to either an unconditional trap or a nop in a later pass.



Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5036
+   "4th operand is not an Immediate");
+int16_t TO = int(SImmOperand4) & 0x1F;
+// when first and second bit of TO not same, swap them

This will be an uninitialized variable when compiled without asserts.



Comment at: 
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll:3-7
+; RUN:   --ppc-asm-full-reg-names -mcpu=pwr8 < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
-; RUN:   -mcpu=pwr7 < %s | FileCheck %s
+; RUN:   --ppc-asm-full-reg-names -mcpu=pwr7 < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
+; RUN:   --ppc-asm-full-reg-names -mcpu=pwr8 < %s | FileCheck %s

This change should just be pre-committed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112285

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

[PATCH] D111434: [PowerPC] PPC backend optimization on conditional trap intrustions

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1011
+  case PPC::TDI:
+  case PPC::TWI: {
+MachineInstr *LiMIA = getVRegDefOrNull((1), MRI);

Seems that we should be able to handle all 4 in the same block:
- Check that both operands are `LI[8]`/`LI[S][8]+ORI[8]` or an immediate
- Set the variables for the three constants
- Determine if this is an unconditional trap or never trap
- Emit the correct instruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111434

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