[PATCH] D149486: [RISCV] Strengthen atomic ordering for sequentially consistent stores

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked 2 inline comments as done.
paulkirth added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:790
+                                          "EnableSeqCstTrailingFence",
+                                          "true",
+                                          "Enable trailing fence for seq-cst store.">;
----------------
craig.topper wrote:
> paulkirth wrote:
> > craig.topper wrote:
> > > paulkirth wrote:
> > > > paulkirth wrote:
> > > > > I think I set the default wrong here
> > > > so .. the behavior I'm observing here seems wrong. Passing `"true"` here actually seems to cause the generated macro to be `GET_SUBTARGETINFO_MACRO(EnableSeqCstTrailingFence, false, enableSeqCstTrailingFence)`, which is what we want, but I would have thought that would only happen if we passed `"false"` here ... am I missing something in how the tablegen is working w/ `SubtargetFeature`?
> > > The value in tablegen is not the default value. It’s the value to represent enabled. I think it can be “true” or an integer/enum. “true” means it’s a bool that starts as false. Anything else is an integer that’s starts at 0.
> > > 
> > > For the integer/enum case you can have multiple features touching the same field that all get maxed together.
> > Ah, thanks. I knew I was missing something in how this was intended to work. I've looked a few times through the tablegen documentation and tried to walk through the relevant definitions in the tablegen files & C++, but I've been unable to figure out exactly where I should have been looking. Could you point me to the relevant bits? 
> I can't find good documentation for this. The best I can find is this https://github.com/llvm/llvm-project/blob/main/llvm/utils/TableGen/SubtargetEmitter.cpp#L1803 where the initialization code for what to do when a feature is enabled is generated.
> 
> If the value is "true" or "false" it's treated as a bool and does a direct assignment. Otherwise it generates a comparison to take the maximum.
> 
> `GET_SUBTARGETINFO_MACRO` is relatively new within the last year or two. In the old says we had to write code in *Subtarget.h to initialize the field. I think we might still need to do that for non-bool features.
> 
> Here's the code for `GET_SUBTARGET_MACRO` that picks the initial value by flipping "true" or "false" SubtargetEmitter::EmitSubtargetInfoMacroCalls. Looks like it skips non-bool features.
Thanks so much for the context, and pointing me to that reference. Tablegen is definitely an area I'd like to be more competent with, but so far my need to interact with it has been fairly limited, since I've mostly only used it to add options to clang, or modify some attributes in the middle-end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149486