[PATCH] D138238: [SROA] For non-speculatable `load`s of `select`s -- split block, insert then/else blocks, form two-entry PHI node

[Bruno De Fraine via Phabricator via llvm-commits]

brunodf added a comment.
Herald added a subscriber: StephenFan.

Hi,

In D138238#3991889 <https://reviews.llvm.org/D138238#3991889>, @lebedev.ri wrote:

> In D138238#3991171 <https://reviews.llvm.org/D138238#3991171>, @uabelho wrote:
>
>> Hello,
>>
>> I noticed that when compiling with gcc I get the following new warning with this patch:
>>
>>   00:14:51 ../include/llvm/Transforms/Scalar/SROA.h:95:7: warning: 'llvm::SROAPass' declared with greater visibility than the type of its field 'llvm::SROAPass::SelectsToRewrite' [-Wattributes]
>>   00:14:51    95 | class SROAPass : public PassInfoMixin<SROAPass> {
>>   00:14:51       |       ^~~~~~~~
>>
>> I guess it's because
>>
>>   using PossiblySpeculatableLoad =
>>       PointerIntPair<LoadInst *, 2, sroa::SelectHandSpeculativity>;
>>   using PossiblySpeculatableLoads = SmallVector<PossiblySpeculatableLoad, 2>;
>>
>> are declared inside
>>
>>   namespace sroa LLVM_LIBRARY_VISIBILITY {
>>
>> but then PossiblySpeculatableLoads is used in the SROAPass member
>>
>>   SmallMapVector<SelectInst *, sroa::PossiblySpeculatableLoads, 8>
>>       SelectsToRewrite;
>>
>> I've no idea if this is anythinig to care about though?
>
> Yeah, @arsenm told me about that. I'm not sure about this diagnostic.
> What really worries me though, is that we don't appear to have GCC `-Werror` bots?

This warning is still present in current builds using GCC, also using recent GCC versions, see this buildbot <https://lab.llvm.org/buildbot/#/builders/104>:

  /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/include/llvm/Transforms/Scalar/SROA.h:96:7: warning: ‘llvm::SROAPass’ declared with greater visibility than the type of its field ‘llvm::SROAPass::SelectsToRewrite’ [-Wattributes]

The warning does seem to point at a genuine problem. The intent of the sroa namespace is to hide details from clients (possibly in other modules), see here <https://github.com/llvm/llvm-project/blob/0515ccc0c48b919dae03edc391304e66cdb75d66/llvm/include/llvm/Transforms/Scalar/SROA.h#L41>.

  /// A private "module" namespace for types and utilities used by SROA. These
  /// are implementation details and should not be used by clients.
  namespace LLVM_LIBRARY_VISIBILITY sroa {

But because the SROAPass has the **field** `SelectsToRewrite`, which contains `sroa::PossiblySpeculatableLoads` values, it is required to know these details to calculate its size, determine its layout, etc.

Would it be an option to put `PossiblySpeculatableLoads` (and its constituents) in another namespace (for example `sroa_detail`), with normal visibility? Or can we store the `SelectsToRewrite` as a unique_ptr rather than as the map vector directly (pimpl pattern)? The latter would allow to remove the details of `PossiblySpeculatableLoads` (and its constituents) from the SROA.h header, but would require to follow an extra indirection to access `SelectsToRewrite`.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138238