[PATCH] D149288: [X86] Introduce a large data threshold for the medium code model
Arthur Eubanks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 25 14:06:31 PDT 2023
aeubanks added inline comments.
================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:168
+ cl::desc("Choose large data threshold for x86_64 medium code model"),
+ cl::init(0));
+ CGBINDOPT(LargeDataThreshold);
----------------
MaskRay wrote:
> aeubanks wrote:
> > MaskRay wrote:
> > > To match GCC, we can use 65536
> > >
> > > Consider applying this to large code model as well.
> > >
> > > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619695.html
> > changes to this value will come in a later patch, and same for making the large code model respect this value
> This seems strange. Introducing this variable and setting it to an appropriate should come in the same patch, no ?
>
> I don't want `clang -mcmodel=medium` emitted data sections for small objects to change from `.data` to `.ldata`, then to `.data` again.
this patch doesn't change the current behavior of the medium code model in clang since a large data threshold of zero treats all data as large, which is the current behavior. I'd rather have the implementation in this patch then a separate small patch to actually change the value, especially since not all of the large data threshold functionality is done yet (e.g. D150297)
================
Comment at: llvm/tools/llc/llc.cpp:599
Target->setCodeModel(*CM_IR);
+ if (auto LDT = codegen::getExplicitLargeDataThreshold())
+ Target->setLargeDataThreshold(*LDT);
----------------
tkoeppe wrote:
> MaskRay wrote:
> > aeubanks wrote:
> > > tkoeppe wrote:
> > > > Can we avoid the implicit check for 0 here and spell out `; LDT > 0`? Does LLVM require C++17?
> > > yeah LLVM uses C++17
> > >
> > > `codegen::getExplicitLargeDataThreshold` returns a `std::optional`, so we're checking if the user explicitly overrode the value, not if the value is 0. we would want to set the value to 0 if the user explicitly requested it
> > Building llvm-project requires c++17 (libomptarget may be an exception).
> >
> > We can use `if (auto LDT = ...; LDT && LDT > 0)` only if the default is 0.
> Ah yes, sorry, I forgot that it's an optional<int>, not just an int. In that case all I meant was for the check to not be implicit, i.e. I'd recommend:
>
> for (auto LDT = ...; LDT.has_vaiue()) {
> // use *LDT
> }
>
> Just so there's one less implicit thing one needs to keep in mind when reading this.
I think this is the standard idiom for checking if a `std::optional` has a value, not calling `has_value()`.
I did change the type to be explicit though, instead of `auto`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149288/new/
https://reviews.llvm.org/D149288
More information about the llvm-commits
mailing list