rengolin closed this revision.
rengolin added a comment.
Committed in r270687.
Repository:
rL LLVM
http://reviews.llvm.org/D20089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.
In http://reviews.llvm.org/D20089#438937, @jmolloy wrote:
> As far as I'm concerned, if you're happy I'm happy. You know this area more
> than me.
Ok, I'll go ahead and commit this, and
jmolloy added a comment.
As far as I'm concerned, if you're happy I'm happy. You know this area more
than me.
Repository:
rL LLVM
http://reviews.llvm.org/D20089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
rengolin added a comment.
LGTM. James? Bradley? Tim?
Repository:
rL LLVM
http://reviews.llvm.org/D20089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
jojo added inline comments.
Comment at: include/llvm/Support/TargetParser.h:173
@@ +172,3 @@
+StringRef getArchName(unsigned ArchKind);
+bool getArchFeatures(unsigned ArchKind, std::vector );
+unsigned getArchAttr(unsigned ArchKind);
rengolin wrote:
> Nitpick,
jojo updated this revision to Diff 58381.
jojo added a comment.
1.include/llvm/Support/TargetParser.h
Move move the declaration of getArchFeatures to a more reasonable place.
Remove unnecessary parameter for getDefaultCPU.
2.lib/Support/TargetParser.cpp
Make adjustments according to
rengolin added a comment.
Hi Jojo,
I've just mapped these changes to the current ARM implementation and they look
correct. I only have two minor comments and I'm happy with it, but I'll let
Bradley have the final review.
Bradley,
If history is of any help, Jojo will have to change a few
jojo added a comment.
> While I agree with Bradley that the repetition is not pretty, I think it will
> expose all issues to make a class design simple and straightforward, once we
> get all the sharp edges out. But we need to know what are the difficulties on
> Clang, llc and the back-ends,
jojo set the repository for this revision to rL LLVM.
jojo changed the visibility of this Differential Revision from "Public (No
Login Required)" to "All Users".
jojo updated this revision to Diff 58205.
jojo added a comment.
1.include/llvm/Support/AArch64TargetParser.def
Format this file.
jojo added inline comments.
Comment at: include/llvm/Support/AArch64TargetParser.def:20
@@ +19,3 @@
+AARCH64_ARCH("armv8-a", AK_ARMV8A, "8-A", "v8", ARMBuildAttrs::CPUArch::v8_A,
+ FK_CRYPTO_NEON_FP_ARMV8, (AArch64::AEK_CRC | AArch64::AEK_CRYPTO |
AArch64::AEK_FP |
jojo added a comment.
> There is an awful lot of duplication/passing through to another class in
> this, it strikes me that this whole thing could benefit from some level of
> inheritance. I think it would be good to have a base class that defines the
> interface and have both ARM/AArch64 (and
jojo removed rL LLVM as the repository for this revision.
jojo changed the visibility of this Differential Revision from "All Users" to
"Public (No Login Required)".
jojo updated this revision to Diff 58071.
jojo added a comment.
1.unsigned llvm::AArch64::getArchAttr(unsigned ArchKind)
rengolin added a comment.
Hi Jojo,
This is looking better, thanks!
While I agree with Bradley that the repetition is not pretty, but I think it
will expose all issues to make a class design simple and straightforward, once
we get all the sharp edges out. But we need to know what are the
jojo added inline comments.
Comment at: lib/Support/TargetParser.cpp:441
@@ +440,3 @@
+ if (Extensions & AArch64::AEK_PROFILE)
+Features.push_back("+spe");
+
bsmith wrote:
> For ARM there is a table that defines these extensions and how they map to
>
jojo added a comment.
Dear Bradley,Renato
Sorry for late reply,I have been on leave the last three days.
Thank you very much for your review and suggestons.I will re pondering the
changes.
Repository:
rL LLVM
http://reviews.llvm.org/D20089
jojo created this revision.
jojo added reviewers: bsmith, jmolloy, rengolin.
jojo added subscribers: llvm-commits, cfe-commits.
jojo set the repository for this revision to rL LLVM.
jojo changed the visibility of this Differential Revision from "Public (No
Login Required)" to "All Users".
Herald
rengolin added a comment.
Jojo,
Thanks for putting this forward. Regarding Bradley's point, it may be clearer
if we go directly with a class based design, but if it changes the ARM part at
the same time (especially testing), than I'd prefer to avoid it. Can you try
this different approach
rengolin added a comment.
In http://reviews.llvm.org/D20089#425542, @bsmith wrote:
> I think that made sense when we only had ARM using this, but not so much now
> since we essentially have two implementations of the same thing.
What about the argument of doing slow and steady changes instead
bsmith added a comment.
In http://reviews.llvm.org/D20089#425541, @rengolin wrote:
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150824/296862.html
>
> One option is to add the duplication, solve all the platform problems first,
> then move to a class based design.
>
> Another is
rengolin added a comment.
In http://reviews.llvm.org/D20089#425525, @bsmith wrote:
> There is an awful lot of duplication/passing through to another class in
> this, it strikes me that this whole thing could benefit from some level of
> inheritance. I think it would be good to have a base
bsmith added a comment.
There is an awful lot of duplication/passing through to another class in this,
it strikes me that this whole thing could benefit from some level of
inheritance. I think it would be good to have a base class that defines the
interface and have both ARM/AArch64 (and any
21 matches
Mail list logo
