[all-commits] [llvm/llvm-project] 794c42: [mlir][LLVMIR] Do not update instMap via assignmen...

[Min-Yih Hsu via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 794c4218a64757dd1cdfb787262d6327000ac5cd
      https://github.com/llvm/llvm-project/commit/794c4218a64757dd1cdfb787262d6327000ac5cd
  Author: Min-Yih Hsu <minyihh at uci.edu>
  Date:   2022-05-04 (Wed, 04 May 2022)

  Changed paths:
    M mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
    A mlir/test/Target/LLVMIR/Import/incorrect-instmap-assignment.ll

  Log Message:
  -----------
  [mlir][LLVMIR] Do not update instMap via assignments to entry references

Inside processInstruction, we assign the translated mlir::Value to a
reference previously taken from the corresponding entry in instMap.
However, instMap (a DenseMap) might resize after the entry reference was
taken, rendering the assignment useless since it's assigning to a
dangling reference. Here is a (pseudo) snippet that shows the concept:
```
// inst has type llvm::Instruction *
Value &v = instMap[inst];
...
// op is one of the operands of inst, has type llvm::Value *
processValue(op);
// instMap resizes inside processValue
...
translatedValue = b.createOp<Foo>(...);
// v is already a dangling reference at this point!
// The following assignment is bogus.
v = translatedValue;
```

Nevertheless, after we stop caching llvm::Constant into instMap, there
is only one case that can cause processValue to resize instMap: If the
operand is a llvm::ConstantExpr. In which case we will insert the
derived llvm::Instruction into instMap.
To trigger instMap to resize, which is a DenseMap, the threshold depends
on the ratio between # of map entries and # of (hash) buckets. More specifically,
it resizes if (# of map entries / # of buckets) >= 0.75.
In this case # of map entries is equal to # of LLVM instructions, and # of
buckets is the power-of-two upperbound of # of map entries. Thus, eventually
in the attaching test case (test/Target/LLVMIR/Import/incorrect-instmap-assignment.ll),
we picked 96 and 128 for the # of map entries and # of buckets, respectively.
(We can't pick numbers that are too small since DenseMap used inlined
storage for small number of entries). Therefore, the ConstantExpr in the
said test case (i.e. a GEP) is the 96-th llvm::Value cached into the
instMap, triggering the issue we're discussing here on its enclosing
instruction (i.e. a load).

This patch fixes this issue by calling `operator[]` everytime we need to
update an entry.

Differential Revision: https://reviews.llvm.org/D124627