Re: [PATCH] D13351: [Power PC] add soft float support for ppc32

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: lib/Driver/Tools.cpp:1477
@@ +1476,3 @@
+  // If unspecified, choose the default based on the platform.
+  if (ABI == ppc::FloatABI::Invalid) {
+ABI = ppc::FloatABI::Hard;

rjmccall wrote:
> hfinkel wrote:
> > Don't need the { } here.
> We don't seem to have a specific style guideline *mandating* the uses of 
> braces around even single-line statements, but please don't *discourage* them.
I don't believe this represents the current consensus convention on this 
matter. We do actively discourage use of unnecessary braces for ifs, fors, etc. 
The exception being that a series of ifs and elses should have braces for all 
of them if any of them need them.

I often point people at the examples here as representative: 
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

You're right, however, we don't formally dictate this policy in our coding 
standards. Do you actively dislike this viewpoint?


http://reviews.llvm.org/D13351



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

Re: [PATCH] D15195: PR4941: Add support for -fno-builtin-foo options.

[hfin...@anl.gov via cfe-commits]

hfinkel added a subscriber: hfinkel.
hfinkel added a comment.

Can you use a StringSet instead of a vector and avoid all (most) of the code 
iterating over the vector of builtins being disabled?



Comment at: lib/Frontend/CompilerInvocation.cpp:147
@@ +146,3 @@
+Values.push_back(FuncName);
+  // FIXME: We could warn about invalid/unsupported -fno-builtin-*.
+}

I'd remove this; there's no need for Clang to know about all functions that the 
optimizer knows about, and thus for which adding the nobuiltin attribute might 
be useful.


http://reviews.llvm.org/D15195



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

Re: [PATCH] D14872: PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2783
@@ -2783,1 +2782,3 @@
+  "the newer semantic is provided here">,
+  InGroup>;
 def warn_transparent_union_attribute_field_size_align : Warning<

Calling this "a semantic" reads oddly to me. This sounds better to me:

  def note_attribute_packed_for_bitfield_offset_changed : Warning<
"the offset assigned to packed bit-field member %0 has changed with GCC 
version 4.4 - "
"the newer offset is used here">,
InGroup>;


Comment at: lib/Sema/SemaDeclAttr.cpp:1040
@@ -1039,3 +1039,3 @@
 // If the alignment is less than or equal to 8 bits, the packed attribute
 // has no effect.
 if (!FD->getType()->isDependentType() &&

This comment is now out of date?


http://reviews.llvm.org/D14872



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

Re: [PATCH] D14872: PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type

[hfin...@anl.gov via cfe-commits]

hfinkel added a subscriber: hfinkel.
hfinkel added a comment.

Please upload this patch with full context, see: 
http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

> Should we add warning about changes in layout for packed bit-fileds of char 
> type? GCC has it "note: offset of packed bit-field ‘b’ has changed in GCC 
> 4.4" will add if needed.


Warning about this is a good idea. We did something similar when we 
(re-)introduced sync_fetch_and_nand, see:

  def warn_sync_fetch_and_nand_semantics_change : Warning<
"the semantics of this intrinsic changed with GCC "
"version 4.4 - the newer semantics are provided here">,
InGroup>;


http://reviews.llvm.org/D14872



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

Re: [PATCH] D13351: [Power PC] add soft float support for ppc32

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: lib/CodeGen/TargetInfo.cpp:3421
@@ -3419,3 +3420,3 @@
 public:
-  PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes ) : DefaultABIInfo(CGT) {}
+  PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes , bool SoftFloatABI) : 
DefaultABIInfo(CGT), IsSoftFloatABI(SoftFloatABI) {}
 

Line too long.


Comment at: lib/Driver/Tools.cpp:1353
@@ -1351,1 +1352,3 @@
 
+  // FIXME: Remove error for ppc64 when soft-float support is added.
+  ppc::FloatABI FloatABI = ppc::getPPCFloatABI(D, Args);

Remove this FIXME. It is not clear this will ever be supported (is there 
actually ppc64 hardware without an FP unit)?


Comment at: lib/Driver/Tools.cpp:1362
@@ +1361,3 @@
+Triple.getArch() == llvm::Triple::ppc64le))
+D.Diag(diag::err_drv_ppc64_softfloat_not_supported);
+

Don't introduce a new error for this. Just reuse the 
diag::err_drv_invalid_mfloat_abi error.


http://reviews.llvm.org/D13351



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

Re: [PATCH] D14871: [Power PC] fix calculating address of arguments on stack for variadic functions

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: lib/CodeGen/TargetInfo.cpp:3547
@@ +3546,3 @@
+// Round up address of argument to alignment
+llvm::Value *overflow_arg_area = OverflowArea.getPointer();
+uint32_t Align = CGF.getContext().getTypeAlignInChars(Ty).getQuantity();

You need to use variable names consistent with our coding convention 
(OverflowArgArea, etc.).


http://reviews.llvm.org/D14871



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

Re: [PATCH] D14200: Make FP_CONTRACT ON default.

[hfin...@anl.gov via cfe-commits]

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

LGTM, thanks!


http://reviews.llvm.org/D14200



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

Re: [PATCH] D13351: [Power PC] add soft float support for ppc32

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

Can you please make sure we produce some sensible error should someone try to 
use soft float with ppc64 (since we don't support that), and add an associated 
test.



Comment at: lib/Driver/Tools.h:739
@@ +738,3 @@
+  Soft,
+  SoftFP,
+  Hard,

We don't seem to support (or even accept) SoftFP. Given that, either remove it 
from the enum or add support for it.


Comment at: lib/Driver/Tools.h:742
@@ +741,3 @@
+};
+   ppc::FloatABI getPPCFloatABI(const Driver , const llvm::opt::ArgList 
);
+}

Unindent this and ppc::FloatABI -> FloatABI


http://reviews.llvm.org/D13351



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

Re: [PATCH] D11182: [OPENMP 4.0] Initial support for 'omp declare reduction' construct.

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: include/clang/Basic/DiagnosticParseKinds.td:995
@@ -994,1 +994,3 @@
+def err_omp_expected_reduction_identifier : Error<
+  "expected identifier or one of the following operators: '+', '-', '*', '&', 
'|', '^', '&&' and '||'">;
 

We're not incredibly consistent here, but I think this reads better if we say:

  '&&', or '||'

instead of:

  '&&' and '||'

(adding the Oxford comma and 'and' -> 'or').


Comment at: lib/AST/Decl.cpp:1463
@@ -1461,1 +1462,3 @@
 
+  // Declare reduction are always replaceable.
+  if (OMPDeclareReductionDecl::classofKind(NewK))

are -> is


http://reviews.llvm.org/D11182



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

Re: [PATCH] D13304: Avoid inlining in throw statement

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D13304#269049, @junbuml wrote:

> I just want to ping one more time to see if there is any objection about the 
> basic idea of this change. If the basic idea itself is acceptable, then I 
> want to find the best way to get idea in.
>
> Please let me know any new suggestion or any opinion about moving this 
> implementation back to backend (maybe in PrunceEH.cpp) with the minimum 
> callee size check to avoid blindly marking noinlines on all callsites. I will 
> be happy to hear any opinion.


It seems like we want two things here:

1. Mark the exception-handling intrinsics as cold
2. Have the inliner not inline things in cold regions

What is preventing us from doing this now?


http://reviews.llvm.org/D13304



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

Re: [PATCH] D13304: Avoid inlining in throw statement

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D13304#276660, @junbuml wrote:

> Did you mean to add the Cold in the exception handling region itself instead 
> of callsite in throw statements ?


We already have BranchProbabilityInfo::calcColdCallHeuristics, and so adding it 
to the callsite should be sufficient. Existing infrastructure should take care 
of the rest.


http://reviews.llvm.org/D13304



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

Re: [PATCH] D13351: [Power PC] add soft float support for ppc32

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: include/clang/Basic/TargetInfo.h:688
@@ -687,1 +687,3 @@
 
+  virtual bool isSoftFloatABI() const {
+return false;

Instead of adding this function, please use the same mechanism as 
X86_32TargetCodeGenInfo and X86_32ABIInfo to feed the soft-float abi 
information through.



Comment at: lib/Basic/Targets.cpp:877
@@ -875,3 +876,3 @@
 
-
+  bool isSoftFloatABI() const override {return IsSoftFloat;}
   StringRef getABI() const override { return ABI; }

Add spaces after { and before }.


Comment at: lib/Basic/Targets.cpp:1072
@@ -1070,1 +1071,3 @@
 
+  auto Feature = std::find(Features.begin(), Features.end(), "+soft-float");
+if (Feature != Features.end()) {

This check can be part of the loop above.


Comment at: lib/Driver/Tools.cpp:1372
@@ +1371,3 @@
+  if (FloatABI != "soft" && FloatABI != "hard") {
+FloatABI = "hard";
+  }

Unless there is a good reason to consider all unknown strings equivalent to 
"hard", please produce an error (and an associated test case).



Comment at: test/Driver/ppc-features.cpp:18
@@ -15,1 +17,3 @@
+// CHECK-SOFTFLOAT: "-target-feature" "+soft-float"
+
 // CHECK: invalid argument '-faltivec' only allowed with 'ppc/ppc64/ppc64le'

Also add a test case with -mhard-float, and both -msoft-float and -mhard-float 
in different orders.
Also add test cases with -mfloat-abi=X


http://reviews.llvm.org/D13351



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

Re: [PATCH] D12979: Avoid inlining in exception handling context

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

You should start a new differential for this, so that you can get a clean 
summary e-mail with a description sent to cfe-commits. There's some overlap, 
but you'll also get potentially-different reviewers here.


http://reviews.llvm.org/D12979



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

Re: [PATCH] D12840: [cfe-dev] Enabling ThreadSanitizer on PPC64(BE/LE) plarforms

[hfin...@anl.gov via cfe-commits]

hfinkel added a subscriber: hfinkel.
hfinkel accepted this revision.
hfinkel added a reviewer: hfinkel.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM, although don't commit until any necessary backend/compiler-rt patches are 
in.


http://reviews.llvm.org/D12840



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

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[hfin...@anl.gov via cfe-commits]

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

In http://reviews.llvm.org/D11815#243031, @hfinkel wrote:

> In http://reviews.llvm.org/D11815#242616, @ahatanak wrote:
>
> > Hal, do you have any thoughts on the points Vasileios brought up? 
> > Currently, many of the targets don't guarantee that the realigned stack is 
> > at least as aligned as the default alignment required by the ABI. Is this 
> > the behavior end-users expect when they use -mstackrealign on the command 
> > line?
>
>
> I don't think this is expected behavior, and sounds like a bug.


To be more specific, my understanding of the use case (which is the use case 
for this I've come across myself), is that you need to compile code for a 
system that does not actually provide the stack alignment that LLVM believes 
should be guaranteed by the current target ABI. This can happen if the 
alignment guarantee has changed in the past, and you need to run on the older 
systems.

Also, on PowerPC, for example, the current LLVM implementation might relaign to 
using a lower-than-target-ABI alignment when we force realignment, but this 
should just be a noop.

In any case, this now LGTM.

> 

> 

> > Fixing this is beyond the initial scope of this patch and probably should 
> > be done in a separate patch, but I want to make sure the patch I'll end up 
> > committing won't make it harder to fix this problem.

> 

> 

> Agreed. I don't see how this makes it harder.



http://reviews.llvm.org/D11815



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

Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D11815#242616, @ahatanak wrote:

> Hal, do you have any thoughts on the points Vasileios brought up? Currently, 
> many of the targets don't guarantee that the realigned stack is at least as 
> aligned as the default alignment required by the ABI. Is this the behavior 
> end-users expect when they use -mstackrealign on the command line?


I don't think this is expected behavior, and sounds like a bug.

> Fixing this is beyond the initial scope of this patch and probably should be 
> done in a separate patch, but I want to make sure the patch I'll end up 
> committing won't make it harder to fix this problem.


Agreed. I don't see how this makes it harder.


http://reviews.llvm.org/D11815



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

Re: [PATCH] D12313: Introduce __builtin_nontemporal_store and __builtin_nontemporal_load.

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D12313#241697, @mzolotukhin wrote:

> Hi Richard, Hal,
>
> Does the patch look good now?


Looks good to me, but please wait for Richard on the changes he requested.

> Thanks,

> Michael



http://reviews.llvm.org/D12313



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

Re: [PATCH] D12313: Introduce __builtin_nontemporal_store and __builtin_nontemporal_load.

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

Thanks, but I still have this question:

 Plus, I don't understand why you're implementing another place in CodeGen 
 that generates IR to load and store scalar values. Can't you enhance 
 EmitLoadOfScalar/EmitStoreOfScalar and then use those functions? If nothing 
 else, you're already missing setting of TBAA metadata and other things that 
 these functions do.



http://reviews.llvm.org/D12313



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

Re: [PATCH] D12313: Introduce __builtin_nontemporal_store and __builtin_nontemporal_load.

[hfin...@anl.gov via cfe-commits]

hfinkel added a comment.

I also like this intrinsic approach better than the type attribute.

The fact that it does not work with builtin vector types, however, is quite 
unfortunate. Plus, I don't understand why you're implementing another place in 
CodeGen that generates IR to load and store scalar values. Can't you enhance 
EmitLoadOfScalar/EmitStoreOfScalar and then use those functions? If nothing 
else, you're already missing setting of TBAA metadata and other things that 
these functions do.


http://reviews.llvm.org/D12313



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

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: lib/Driver/Tools.cpp:4232
@@ +4231,3 @@
+   false))
+CmdArgs.push_back(Args.MakeArgString(-force-align-stack));
+

ahatanak wrote:
 echristo wrote:
  hfinkel wrote:
   echristo wrote:
hfinkel wrote:
 The code below for OPT_mstackrealign uses -mstackrealign as the name 
 of the backend option too. Why not do the same for OPT_mstackrealign 
 (use -mstackrealign as the name of the backend option) instead of 
 inventing a new flag name -force-align-stack?
In general we don't do that. But I also don't want this to use a 
backend option anyhow, why are we doing that here once we have the 
attribute?
   It's not a backend option, this is the flag being passed from the driver 
   to clang -cc1.
   
  Aha. I must have misread. Then I totally agree :)
 I believe the confusing part here is that the CC1 option -mstackrealign in 
 the code below indicates stack realignment is allowed, but not necessarily 
 forced (see the definition of StackRealignment in CodeGenOptions.def). This 
 is different from the driver option options::OPT_mstackrealign, which 
 indicates stack alignment should be forced.
 
 Does that answer your question?
Yes, but this somehow makes things seem even more broken. As I understand it, 
we have two underlying CodeGen options here:

 1. May the backend realign the stack if it thinks that it should? [Mainly 
because it has some overaligned local variable to put on the stack]. This is on 
by default.
 2. Must the backend realign all functions. This is off by default.

GCC has an option -mstackrealign documented to mean only (2). But we currently 
use its inverse (-mno-stackrealign) to mean the inverse of (1). Frankly, (1) 
seems like a debugging option, and I don't see why we are exposing it to users. 
If we have overaligned locals, than the backend should realign the stack. 
Always. Otherwise the code is broken. Then we can use -mstackrealign for its 
intended purpose of only meaning (2), and -mno-stackrealign the inverse of (2).



http://reviews.llvm.org/D11815



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

Re: [PATCH] D11815: Pass subtarget feature force-align-stack

[hfin...@anl.gov via cfe-commits]

hfinkel added a subscriber: hfinkel.
hfinkel requested changes to this revision.
hfinkel added a reviewer: hfinkel.
hfinkel added a comment.
This revision now requires changes to proceed.

As I've said in the review for http://reviews.llvm.org/D11814, this should be 
added to TargetOptions, and controlled accordingly.


http://reviews.llvm.org/D11815



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