[PATCH] D143708: [RISCV] Support emulated TLS

Mon Mar 20 22:02:12 PDT 2023

jrtc27 added inline comments.

================
Comment at: llvm/test/CodeGen/RISCV/emutls.ll:10
+ at external_x = external thread_local global i32, align 8
+ at external_y = thread_local global i8 7, align 2
+ at internal_y = internal thread_local global i64 9, align 16
----------------
vit9696 wrote:
> jrtc27 wrote:
> > vit9696 wrote:
> > > jrtc27 wrote:
> > > > vit9696 wrote:
> > > > > jrtc27 wrote:
> > > > > > This isn’t external?
> > > > > Yes, due to internal_y with the same name.
> > > > Huh? You can change y to something else... and you probably should, having two y-based variables is unnecessarily confusing when you could just use z.
> > > > 
> > > > I forget if initialised globals without a specified linkage are in fact technically regarded as external, but we don't let you write external there in the IR so it seems a bit confusing to put it in the variable name.
> > > This tests whether it works correctly when internal and external symbols are present.
> > > 
> > > Either way, I am fine with this change if you insist. Since it is the last change requested and I do not have commit access, could you please handle it on your own and push to master? Thanks!
> > I would prefer the test as committed go through proper pre-commit review
> Actually wait.
> 
> ```
> @external_y = thread_local global i8 7, align 2
> ```
> 
> `external thread_local global i8` does not allow size to be specified here. The test is correct as is.
That is not my point. My point is that the name says external_y, which does not match the fact that the RHS doesn't have (and as you correctly state, does not support) external.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143708