[PATCH] D110681: [X86][ISel] Lowering llvm.thread.pointer

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:26724
+      Value *Ptr = Constant::getNullValue(Type::getInt8PtrTy(
+          *DAG.getContext(), Subtarget.is64Bit() ? 257 : 256));
+      return DAG.getLoad(PtrVT, dl, DAG.getEntryNode(),
----------------
X86::FS, X86::GS?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:26726
+      return DAG.getLoad(PtrVT, dl, DAG.getEntryNode(),
+                         DAG.getIntPtrConstant(0, dl), MachinePointerInfo(Ptr));
+    } else {
----------------
Can we use this method directly? https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/MachineMemOperand.h#L63


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:26727
+                         DAG.getIntPtrConstant(0, dl), MachinePointerInfo(Ptr));
+    } else {
+      report_fatal_error("OS doesn't support __builtin_thread_pointer() yet.");
----------------
We don't use `else` after `return`.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:26728
+    } else {
+      report_fatal_error("OS doesn't support __builtin_thread_pointer() yet.");
+    }
----------------
Not sure we have to limit it to Linux here. I found other targets don't do this check.
Besides, we already have check in https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L5040
If we don't support tls, we won't go here. But if we do, we should return the right thread pointer.


================
Comment at: llvm/test/CodeGen/X86/thread_pointer.ll:48
+  %1 = bitcast i8* %0 to i32*
+  %idxprom = sext i32 %i to i64
+  %arrayidx = getelementptr inbounds i32, i32* %1, i64 %idxprom
----------------
Do we have to use i64? Especially we are testing X32 and X86 at the same time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110681/new/

https://reviews.llvm.org/D110681