intel / intel-graphics-compiler

Other
594 stars 155 forks source link

genx_volatile-attributed globals pointers can indeed be used by bitcast (constant expressions or in instruction form) before being used in genx.vload/vstore. #325

Open lsatanov opened 5 months ago

lsatanov commented 5 months ago

Hi, @KorovinVlad.

The commit https://github.com/intel/intel-graphics-compiler/commit/4f6d3301cfd60e609a25a608b5b702ab8bd923b9 introduces checks for genx_volatile-attributed globals users.

One detail is that they can indeed be used by bitcast constant expression or instruction bitcasts before genx.vload/vstore. E.g. pseudocode:

v = genx.vload(bitcast(@g*... to ...))

OR

ptr = bitcast(@g*... to ...) v = genx.vload(ptr)

For this particular reason e.g. genx::getAVLoadSrcOrNull(...) routine will do the scan through bitcasts in reverse direction, up the def-use chain using getBitCastedValue(...): const auto *Src = getBitCastedValue(I->getOperand(0))

N.B. present LIT-tests do not include this kind of usage, but it can be found during real world workloads compilation, so, most probably it'd be useful to include those cases in the LIT test for GenXVerify.cpp.

Therefore here

https://github.com/intel/intel-graphics-compiler/blob/4f6d3301cfd60e609a25a608b5b702ab8bd923b9/IGC/VectorCompiler/lib/GenXCodeGen/GenXVerify.cpp#L72

genx::peelBitCastsWhileSingleUserChain(...) should be done before casting to an Instruction, like this:

auto InvalidUser = llvm::find_if(GV.users(), [](const User *U) {
  const auto *I = dyn_cast<Instruction>(genx::peelBitCastsWhileSingleUserChain(U));
  return !I || !(genx::isAVStore(I) || genx::isAVLoad(I));
});

Additionally, it would be also nice to see all of the wrong usages in IR, not only the first one, hence the code could look like this:


  auto IsInvalidUser = [](const User *U) {
    const auto *I = dyn_cast<Instruction>(genx::peelBitCastsWhileSingleUserChain(U));
    return !I || !(genx::isAVStore(I) || genx::isAVLoad(I));
  };

  for(const auto &U: GV.users())
    ensure(!IsInvalidUser(*U),
         "a global volatile variable may only be used in genx.vload/genx.vstore"
         "intrinsics or volatile load/store instructions, optionally preceeded"
         " by a pointer type bitcast",
         U);