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:
> > rjmccall
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 ne
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
hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.
One minor comment, otherwise LGTM.
Comment at: lib/Driver/Tools.cpp:1477
@@ +1476,3 @@
+ // If unspecified, choose the default based on the platform.
+ if (ABI == ppc::Flo
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" rea
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:
hfinkel added inline comments.
Comment at: lib/CodeGen/TargetInfo.cpp:3421
@@ -3419,3 +3420,3 @@
public:
- PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes &CGT) : DefaultABIInfo(CGT) {}
+ PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes &CGT, bool SoftFloatABI) :
DefaultABIInfo(CGT), IsSoftF
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();
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/list
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,
hfinkel added auditors: rjmccall.
hfinkel added a comment.
John is the right auditor for this, not me.
Users:
hfinkel (Author, Auditor)
3.7-release (Auditor)
cfe-commits (Auditor)
tstellarAMD (Auditor)
rjmccall (Auditor)
http://reviews.llvm.org/rL246882
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_32A
hfinkel added a comment.
In http://reviews.llvm.org/D13802#275471, @chandlerc wrote:
> In http://reviews.llvm.org/D13802#274847, @ABataev wrote:
>
> > Hi Chandler, thanks for the review.
> >
> > In http://reviews.llvm.org/D13802#272053, @chandlerc wrote:
> >
> > > I've also had one test fail, and
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
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 le
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 '||'">;
-
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
__
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
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?
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
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
_
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
> el
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:
> > > > hf
hfinkel added inline comments.
Comment at: lib/Driver/Tools.cpp:4232
@@ +4231,3 @@
+ false))
+CmdArgs.push_back(Args.MakeArgString("-force-align-stack"));
+
echristo wrote:
> hfinkel wrote:
> > The code below for OPT_mstackrealign uses -mstac
hfinkel added inline comments.
Comment at: lib/Driver/Tools.cpp:4232
@@ +4231,3 @@
+ false))
+CmdArgs.push_back(Args.MakeArgString("-force-align-stack"));
+
The code below for OPT_mstackrealign uses -mstackrealign as the name of the
backend
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 sca
hfinkel added a subscriber: hfinkel.
hfinkel added a comment.
In http://reviews.llvm.org/D11932#222515, @ismail wrote:
> Tests pass but, now when I tried to compile a file with -fopenmp I get:
>
> LINK: fatal error LNK1104: cannot open file 'libomp.lib'
>
> libomp.lib does exist in "C:\Program Fi
hfinkel added a comment.
In http://reviews.llvm.org/D11815#224169, @echristo wrote:
> No, RESET_OPTION isn't the right way to do this either. For exactly this sort
> of reason. You can't actually represent all of the code and options this way
> in the IR. If you can't do that then it's a non-st
hfinkel added a comment.
In http://reviews.llvm.org/D11815#224161, @echristo wrote:
> Hi Hal,
>
> No, TargetOptions is exactly the wrong place to handle this due to wanting to
> have various functions compiled with and without a force aligned stack at the
> IR level that might not hold up at LT
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 con
hfinkel added a subscriber: hfinkel.
Comment at: lib/CodeGen/CGClass.cpp:1832
@@ +1831,3 @@
+ // Generate vtable assumptions if we are calling dynamic class ctor
+ // and we are not in another ctor.
+ if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
I think th
hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.
LGTM.
http://reviews.llvm.org/D11739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe
hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.
LGTM, but obviously any necessary CodeGen changes must be done first (or at the
same time).
http://reviews.llvm.org/D11738
___
cfe-commits mai
33 matches
Mail list logo
