[PATCH] D52074: [PowerPC] [Clang] Add vector int128 pack/unpack builtins

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

I will commit the patch for you.


Repository:
  rC Clang

https://reviews.llvm.org/D52074



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

[PATCH] D80723: [PowerPC] Convert vec_splats functions to macros

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: clang/lib/Headers/altivec.h:13670
+  )
+#elif defined(__VSX__)
+#define vec_splats(N) \

I am not sure if this is by intention. It is not semantics the same with this 
change. Before the change, if VSX is off, and POWER8_VECTOR && __powerpc64__ is 
on, vector signed/unsigned long long, signed/unsigned __int128 is not a valid 
candidate of vec_splats. But with this patch, they are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80723



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

[PATCH] D80723: [PowerPC] Convert vec_splats functions to macros

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: clang/lib/Headers/altivec.h:13670
+  )
+#elif defined(__VSX__)
+#define vec_splats(N) \

vddvss wrote:
> steven.zhang wrote:
> > I am not sure if this is by intention. It is not semantics the same with 
> > this change. Before the change, if VSX is off, and POWER8_VECTOR && 
> > __powerpc64__ is on, vector signed/unsigned long long, signed/unsigned 
> > __int128 is not a valid candidate of vec_splats. But with this patch, they 
> > are.
> No intention to change semantics. But AFICT, we throw an error if 
> POWER8_VECTOR is on and VSX is off: 
> https://github.com/llvm/llvm-project/blob/master/clang/lib/Basic/Targets/PPC.cpp#L222
Hmm, we are making assumption that, POWER8_VECTOR enables the VSX, and it is 
true. Thank you for pointing out this.



Comment at: clang/test/CodeGen/pr44276.c:3
+// REQUIRES: powerpc-registered-target
+// RUN: %clang -S -emit-llvm -target powerpc64-unknown-unknown -mcpu=pwr8 %s 
-o - | FileCheck %s
+

The assembly output is not your test point. How about doing it as this:
```
// RUN: %clang_cc1 -S -emit-llvm -triple powerpc64-unknown-unknown -target-cpu 
pwr8 %s
// expected-no-diagnostics
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80723



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

[PATCH] D84622: [PowerPC] Implement Vector Extract Low/High Order Builtins in LLVM/Clang

[Qing Shan Zhang via Phabricator via cfe-commits]

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

LGTM. But please hold on for one more days to see if there is other comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84622

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

[PATCH] D80952: [FPEnv][Clang][Driver][WIP] Disable constrained floating point on targets lacking support.

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

The PowerPC backend is also adding the constraint float point support and 
almost done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80952



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

[PATCH] D80723: [PowerPC] Convert vec_splats functions to macros

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

It LGTM now except one comment on the test. And it seems that, we still have 
many other builtins implementation that didn't use the _Generic.




Comment at: clang/test/CodeGen/ppc-emmintrin.c:1
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // REQUIRES: powerpc-registered-target

So, why this line of comments is removed ? It seems that, the old test was 
generated by the script while the new one isn't. I expect both should generate 
by script. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80723



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

[PATCH] D81355: [PowerPC] Enable -fstack-clash-protection option for ppc64

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.
Herald added a subscriber: wuzish.

Shouldn't this be the last patch to commit after the backend supporting this 
feature ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81355



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

[PATCH] D81355: [PowerPC] Enable -fstack-clash-protection option for ppc64

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang accepted this revision.
steven.zhang 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/D81355/new/

https://reviews.llvm.org/D81355



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

[PATCH] D82020: PowerPC-specific builtin constrained FP enablement

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14069
+  BuiltinID == PPC::BI__builtin_vsx_xvrspim)
+ID = Intrinsic::floor;
+  else if (BuiltinID == PPC::BI__builtin_vsx_xvrdpi ||

Can we do it like this to avoid the duplicate if statement ?
```
if (...)
   ID = Builder.getIsFPConstrained() ? 
Intrinsic::experimental_constrained_floor : Intrinsic::floor;
 ...
 return Builder.getIsFPConstrained() ? Builder. CreateConstrainedFPCall() : 
Builder.CreateCall();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82020



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

[PATCH] D82020: PowerPC-specific builtin constrained FP enablement

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

LGTM overall with some minor comments.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14238
  BuiltinID == PPC::BI__builtin_vsx_xvrspi)
-  ID = Intrinsic::round;
+  ID = Builder.getIsFPConstrained() ? 
Intrinsic::experimental_constrained_round : Intrinsic::round;
 else if (BuiltinID == PPC::BI__builtin_vsx_xvrdpic ||

line too long. Please format the code with clang-format. 



Comment at: clang/test/CodeGen/builtins-ppc-fpconstrained.c:4
+// RUN: -disable-O0-optnone -Wall -Wno-unused -Werror -emit-llvm %s -o - | \
+// RUN: FileCheck --check-prefix=CHECK-UNCONSTRAINED -vv %s
+// RUN: %clang_cc1 -triple powerpc64le-gnu-linux -target-feature +vsx \

Do we have to specify option "-disable-O0-optnone -Wall -Wno-unused -Werror" ?


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

https://reviews.llvm.org/D82020



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

[PATCH] D82020: PowerPC-specific builtin constrained FP enablement

[Qing Shan Zhang via Phabricator via cfe-commits]

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

LGTM now and thank you for doing this. Please hold on for 2-3 days in case 
others have comments. And thank you for pointing a potential issue of the 
folding of fneg + fma. We will take a look later.


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

https://reviews.llvm.org/D82020



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

[PATCH] D81836: [PowerPC][Power10] Implement Vector Permute Builtins in LLVM/Clang

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

This is a huge patch. I suggest you to split them into small ones for each kind 
of builtins, so that, it would be more easy to review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81836



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

[PATCH] D82431: [PowerPC][Power10] Implement Test LSB by Byte Builtins in LLVM/Clang

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: llvm/test/MC/Disassembler/PowerPC/vsx.txt:2
 # RUN: llvm-mc --disassemble %s -triple powerpc64-unknown-linux-gnu -mcpu=pwr7 
| FileCheck %s
+# RUN: llvm-mc --disassemble %s -triple powerpc64-unknown-linux-gnu \
+# RUN:   -mcpu=pwr10 | FileCheck %s --check-prefix=CHECK-P10

So, this is for bigendian, where is the little endian disassemble tests ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82431



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

[PATCH] D80952: [FPEnv][Clang][Driver] Disable constrained floating point on targets lacking support.

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.h:86
+
+HasStrictFP = true;
   }

nemanjai wrote:
> I don't think we need this for now. Close is not quite there. @steven.zhang I 
> would prefer that we initially turn this off and only flip it on once the 
> support is complete.
> Also, is the support that is currently under development for both 32 and 64 
> bit architectures? If it is 64 bit only, then we can enable it only there 
> once it is done.
Yes, it hasn't been finished yet. Agree that we can turn on it later when it is 
done.


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

https://reviews.llvm.org/D80952



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

[PATCH] D80020: [PowerPC] Add support for -mcpu=pwr10 in both clang and llvm

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.cpp:272
 .Default(false);
 
   Features["qpx"] = (CPU == "a2q");

Do we miss to define this macro ?
```
 __POWER10_VECTOR__
```



Comment at: llvm/lib/Target/PowerPC/PPC.td:211
+ "true",
+ "Enable instructions added in ISA 3.1.">;
 def FeatureP9Altivec : SubtargetFeature<"power9-altivec", "HasP9Altivec", 
"true",

Nit: no ending dot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80020



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

[PATCH] D80020: [PowerPC] Add support for -mcpu=pwr10 in both clang and llvm

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.cpp:272
 .Default(false);
 
   Features["qpx"] = (CPU == "a2q");

steven.zhang wrote:
> Do we miss to define this macro ?
> ```
>  __POWER10_VECTOR__
> ```
Well, pls ignore this comments as that macro should be added when p10 vector 
feature is added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80020



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

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables.cpp:103
+  // Expect no warning given here.
+  Color color;
+  // Expect no warning given here.

Technical speaking, we should warn about such case, ad the color is undefined 
now. Can we initialize it to the lowerest value of the enum type ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431

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

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

In D106431#2892866 , @MTC wrote:

> In D106431#2892859 , @whisperity 
> wrote:
>
>> Is this the right decision to make, conceptually? It will leave the variable 
>> uninitialised still, and reading such an uninit variable is still an issue, 
>> even if it is an enum.
>
> Yeah, that's right. However, it's much more difficult to give enum an initial 
> value than an integer.
>
>> Could we consider the alternative of warning the user about the 
>> uninitialized variable, just not offering an automatic (and potentially bad 
>> / incorrect) fix?
>
> Make sense, we (ByteDance) are also hesitating whether we should provide 
> automatic repair for uninitialized variables, because automatic fix may 
> change the program semantics.

This is the same as what we did for integer in essential. We are changing an 
uninitialized value to the default one, which makes sense as uninitialized 
means it could be anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431

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

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

In D106431#2907688 , @whisperity 
wrote:

> In D106431#2907002 , @Sockke wrote:
>
>> Any thoughts?  : )
>
> First, let's first fix that we should still warn for the uninitialised `enum` 
> case, without a FixIt. That's the issue at hand, right now, Clang-Tidy 
> generates, as you identified, broken output. We can discuss the later steps 
> after this is fixed. Please implement this logic, and update the patch, so we 
> have a snapshot of how that would look like and the thing working.

+1

> Afterwards, as Aaron suggested:
>
> In D106431#2896441 , @aaron.ballman 
> wrote:
>
>> for enumerations, we could issue up to two fix-its on a note, one for the 
>> first and one for the last enumerator in an enumeration (and if the enum 
>> only contains one enumerator, there's only one fix-it to generate which 
>> suggests it could be on the warning rather than a note, but that seems like 
>> a lot of trouble for an unlikely scenario). However, I don't recall how 
>> clang-tidy interacts with fix-its on notes off the top of my head, so I'm 
>> making an assumption that clang-tidy's automatic fixit applying mode handles 
>> notes the same way as clang and we should double-check that assumption.
>
> This might require nontrivial changes to the check's code, and investigating 
> the potential problem with automated fix-it application when multiple 
> conflicting fix-its are given on a **note** (not a //warning// line).
>
> I think we all would be fine with only doing the first step (reintroduce the 
> warning, without a fixit) in this patch, so it can be merged and hit the the 
> mainline code quickly. The next step can be its own patch. 🙂
>
> 
>
> In addition, I wager that adding a line about this fix to the release notes 
> at `clang-tools-extra/docs/ReleaseNotes.rst` is a good option, I can imagine 
> people having turned this check off due to it being broken on enums.

I think, the intention of the auto fix for such case is to make the program's 
behavior well-defined, not with some uninitialize value that cannot be 
predicted. It is ok as far as we select any enumerator to initialize it from my 
understanding. We should separate it into anther patch of cause.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431

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

[PATCH] D82725: [PowerPC] Implement Move to VSR Mask builtins in LLVM/Clang

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang accepted this revision.
steven.zhang 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/D82725/new/

https://reviews.llvm.org/D82725

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

[PATCH] D88278: [PowerPC] Add builtins for xvtdiv(dp|sp) and xvtsqrt(dp|sp).

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2584
 
+// Vector test software functions.
+def : Pat<(i32 (int_ppc_vsx_xvtdivdp v2f64:$A, v2f64:$B)),

Vector test for software divide and sqrt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88278

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

[PATCH] D85874: [PowerPC] Add readflm/setflm intrinsics to Clang

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang accepted this revision.
steven.zhang 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/D85874/new/

https://reviews.llvm.org/D85874

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

[PATCH] D82726: [PowerPC] Implement Vector Count Mask Bits builtins in LLVM/Clang

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang accepted this revision.
steven.zhang 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/D82726/new/

https://reviews.llvm.org/D82726

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

[PATCH] D82725: [PowerPC] Implement Move to VSR Mask builtins in LLVM/Clang

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10054
+
+  case Intrinsic::ppc_altivec_mtvsrbm: {
+// The llvm.ppc.altivec.mtvsrbm intrinsic can correspond to two different

Can we handle this inside the .td ? i.e. change the definition of the instr as:
```
  def MTVSRBMI : DXForm<4, 10, (outs vrrc:$vD), (ins u8imm64:$D),
"mtvsrbmi $vD, $D", IIC_VecGeneral,
[(set v16i8:$vD,
  (int_ppc_altivec_mtvsrbm imm:$D))]>;
```
And add the missing u8imm64 as what we did for u16imm64 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82725

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

[PATCH] D93377: [Clang] Add __ibm128 type to represent ppc_fp128

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:2668
   // ::= g  # __float128
+  // ::= g  # __ibm128
   // UNSUPPORTED:::= Dd # IEEE 754r decimal floating point (64 bits)

This is a bit confusing as the 'g' is for both __float128 and __ibm128. I know 
that PowerPC will mangle the __float128 as "u9__ieee128", and "g" for __ibm128. 
But c++filt demangle the "g" as __float128 which seems not right. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93377

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

[PATCH] D91279: [PowerPC] DForm instructions should be preferred when using zero register

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:418
+  // should prefer D-form if LXVX / STXVX uses a ZERO or ZERO8
+  if (MI.getOpcode() == PPC::LXVX || MI.getOpcode() == PPC::STXVX) {
+LLVM_DEBUG(dbgs() << "Replacing LXVX/STXVX REG, 0 with LXV/STX\n");

Some comments for current implementation.
1. Can we do this inside TII->convertToImmediateForm()? Notice that we have 
statistic data for such convert. Though it now handles x-load + addi -> d-load, 
it could be extended to handle x-load, 0 -> d-load.
2. I am not sure why just handle the lxvx. We have quite a lot x-form. 
ImmInstrInfo contains such information and can we query it to do the 
transformation instead of enum the opcode?



Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:421
+// if it's Frame index we should not apply the transformation
+if (!MI.getOperand(1).isFI() &&
+(MI.getOperand(1).getReg() == PPC::ZERO ||

check isReg()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91279

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

[PATCH] D88278: [PowerPC] Add builtins for xvtdiv(dp|sp) and xvtsqrt(dp|sp).

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang accepted this revision.
steven.zhang added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88278

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

[PATCH] D88278: [PowerPC] Add builtins for xvtdiv(dp|sp) and xvtsqrt(dp|sp).

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

I think some follow up is needed to optimize the code sequence for

  cr = vec_test_div
  if (rotate_and_mask(cr, 62))
...

For now, we will copy the cr to gpr, and shift it to 61-64 bit,then, extract 
the bit and then compare to 0. the shift is not needed. Please double confirm 
to see if the peephole handle it well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88278

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

[PATCH] D92329: [PowerPC][Clang] Remove QPX support

[Qing Shan Zhang via Phabricator via cfe-commits]

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

LGTM as I grep the whole repo with this patch applied, no QPX any more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92329

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

[PATCH] D90213: [PowerPC] [Clang] Enable float128 feature on P9 by default

[Qing Shan Zhang via Phabricator via cfe-commits]

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

LGTM as it seems the missing part. Please also fix the error message if with 
Power8.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90213

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

[PATCH] D90208: [PowerPC] [Clang] Define macros to identify quad-fp semantics

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang accepted this revision.
steven.zhang added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Basic/Targets/PPC.cpp:122
   if (LongDoubleWidth == 128) {
 Builder.defineMacro("__LONG_DOUBLE_128__");
 Builder.defineMacro("__LONGDOUBLE128");

Can you please double check if we need to define these two macros if PPC IEEE 
long double enabled ? And also double check if they can be controlled by 
options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90208

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

[PATCH] D90208: [PowerPC] [Clang] Define macros to identify quad-fp semantics

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

Sorry, accept the revision by mistake. Please hold on until my comments 
addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90208

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

[PATCH] D90208: [PowerPC] [Clang] Define macros to identify quad-fp semantics

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

LGTM now and thank you for the double check. But please hold on for some days 
in case someone has concern on enabling the new macros(we are enabling the 
__LONG_DOUBLE_IBM128__ by default now).


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

https://reviews.llvm.org/D90208

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

[PATCH] D92815: [PowerPC] [Clang] Enable float128 feature on VSX targets

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang accepted this revision.
steven.zhang added a comment.

LGTM now. But please hold on for at least one week to see if @nemanjai has 
concern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92815

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

[PATCH] D92935: Introduce support for PowerPC devices with an Embedded Floating-point APU version 2 (efpu2)

[Qing Shan Zhang via Phabricator via cfe-commits]

steven.zhang added a comment.

Some code style comments.




Comment at: clang/include/clang/Driver/Options.td:2603
 def mno_spe : Flag<["-"], "mno-spe">, Group;
+def mefpu2 : Flag<["-"], "mefpu2">, Group;
 def mabi_EQ_vec_extabi : Flag<["-"], "mabi=vec-extabi">, Group, 
Flags<[CC1Option]>,

Maybe, clang/docs/ClangCommandLineReference.rst need to be updated also ?



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:707
 
+bool hasEFPU2() const;
+

I think we don't need such wrap but just call Subtarget.hasEFPU2() directly.



Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:103
   bool HasSPE;
+  bool HasEFPU2;
   bool HasVSX;

Miss to initialize it ?


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

https://reviews.llvm.org/D92935

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