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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:790
+                                          "EnableSeqCstTrailingFence",
+                                          "true",
+                                          "Enable trailing fence for seq-cst store.">;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149486