[PATCH] D17092: [X86] Add -mseparate-stack-seg

[Michael LeMay via cfe-commits]

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