[SYCL][NFC] Extract specialization constant's processing from sycl-post-link to SYCLPostLink library.

[maksimsab]

This change allows later reuse of specialization constant's processing in the New Offloading Model and sycl-jit.

[maksimsab]

The interface of a functionhandleSpecializationConstants is controversial. It allows to invoke the function in the following manner if you don't need to generate new modules with spec constants replaced by default values:

SmallVector<ModuleDesc> MDs;
Modified |= handleSpecializationConstants(MDs, Mode);

If you have a better alternative for the function's interface let me know.

[maarquitos14]

Looks good overall, just a few nits.

[asudarsa]

Overall looks good. Minor changes requested.

Thanks

[maksimsab]

Hi @intel/dpcpp-tools-reviewers

Could, please, someone take a look at this PR?

[sarnex]

is there a default value we can use instead of having it being optional? maybe like not_specified or something?

[maksimsab]

Would it be better to have not_specified value? We will have to check that value and return errors deeper in lowering.

[sarnex]

personally i would prefer that

[maksimsab]

What is the expected behaviour in the following snippet then would be?

  ModulePassManager MPM;
  ModuleAnalysisManager MAM;
  SpecConstantsPass SCP(SpecConstantsPass::HandlingMode::not_specified);
  MAM.registerPass([&] { return PassInstrumentationAnalysis(); });
  MPM.addPass(std::move(SCP));

  PreservedAnalyses Res = MPM.run(NewMD->getModule(), MAM);

[YuriPlyakhin]

@sarnex , @maksimsab , I guess the answer to Nick's question in this thread is: there is no default value currently that can be used instead of having SCMode to be optional.

This PR is about extracting existing spec const processing to a library. Updating SpecConstantsPass and introducing new HandlingMode is out of scope of this PR. We can consider it as a future refactoring, but it should not hold this PR from merging.

[sarnex]

it's not worth blocking the PR over, but i'm not sure i understand, the old code does not seem to use optional for the handling mode, so im wondering why we need it in this NFC change

[YuriPlyakhin]

That is a good question! The reason is how logic was split between sycl-post-link and SYCLPostLink library.

The info about spec const mode comes from cl::opt option in sycl-post-link tool, which is:
cl::opt<SpecConstLowerMode> SpecConstLower and is cl::Optional.

Since in the old code SpecConstLower is global for that file and the logic which depends on its optionality is in the same file, we did not need additional optional variable. So there is code like:

if (SpecConstLower.getNumOccurrences() == 0)
    return false;

So, semantically, old code used optional, just not std::optional.

In the new code, we need to keep optional behavior and pass this information somehow to the library function. It seems using std::optional is the closest way to keep the original semantics.

The alternative (to keep the code as close to the original as possible) that I see is to pass cl::opt<SpecConstLowerMode> SpecConstLower as parameter, instead of std::optional<SpecConstantsPass::HandlingMode> Mode.
That doesn't look better to me...

What do you think?

[maksimsab]

Updating SpecConstantsPass and introducing new HandlingMode is out of scope of this PR.

That is correct. Introducing default value would lead to many changes in SpecConstsPass which worth a separate patch.

[sarnex]

i agree that calling getNumOccurences() == 0 is similar to checking if the optional is empty. so yeah feel free to merge the pr and address this in a follow up

[maarquitos14]

Is this already covered by current testing or should we add new testing?

[maksimsab]

@maarquitos14 Yes, we have plenty of sycl-post-link's testing. Later this testing should be migrated for New Offloading Model's tools.

[github-actions]

@intel/llvm-gatekeepers please consider merging

[YuriPlyakhin]

LGTM

[maksimsab]

@intel/llvm-gatekeepers Can we merge that please?