mlemay-intel added a comment.
Thank you for your feedback!
================
Comment at: lib/CodeGen/TargetInfo.cpp:1569
@@ +1568,3 @@
+ CGF.getTarget().getTargetOpts().Features;
+ if (std::find(TargetFeatures.begin(), TargetFeatures.end(),
+ "+separate-stack-seg") != TargetFeatures.end()) {
----------------
delena wrote:
> Hi,
> I'm not clang reviewer at all and you can ignore my comment.
>
> Searching string looks strange for me. I suppose that this string should be
> defined in another form. Something like
> options::OPT_separate_stack_seg.
>
I haven't quite found an example of how an option like this should be handled.
String comparisons of this form are performed in clang/lib/Basic/Targets.cpp,
but there they are used to initialize variables. Furthermore, all of those
tests are for CPU features, whereas this one is for a particular segment
configuration.
================
Comment at: lib/CodeGen/TargetInfo.cpp:1577
@@ +1576,3 @@
+ CGF.Builder.CreatePtrToInt(Addr.getPointer(), CGF.IntPtrTy);
+ llvm::Value *PtrInStackSeg = CGF.Builder.CreateIntToPtr(PtrAsInt,
+
DirectTy->getPointerTo(258));
----------------
delena wrote:
> You should not use 258 as a constant. It should be defined somewhere as
> address space enum.
I agree with this principle. However, the address space numbers for FS and GS
are listed in the Clang documentation [1] and are used as literals in LLVM
code. Thus, I think this patch is consistent with existing usage of address
space numbers.
[1]
http://clang.llvm.org/docs/LanguageExtensions.html#memory-references-off-the-gs-segment
================
Comment at: lib/CodeGen/TargetInfo.cpp:1578
@@ +1577,3 @@
+ llvm::Value *PtrInStackSeg = CGF.Builder.CreateIntToPtr(PtrAsInt,
+
DirectTy->getPointerTo(258));
+ return Address(PtrInStackSeg, Addr.getAlignment());
----------------
delena wrote:
> This line alignment does not match LLVM style.
I'll fix it.
================
Comment at: lib/CodeGen/TargetInfo.cpp:1580
@@ +1579,3 @@
+ return Address(PtrInStackSeg, Addr.getAlignment());
+ }
+
----------------
delena wrote:
> Again, not sure that I'm right. You are trying to create addressspacecast. Is
> it the right way to create ptrtoint + inttoptr?
I first attempted to create an address space cast, but LLVM disallows that. I
think it is necessary to perform the conversion through an int, but I am not
entirely sure of that.
http://reviews.llvm.org/D17092
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
