[mlir][LLVMIR] Do not update instMap via assignments to entry references
authorMin-Yih Hsu <minyihh@uci.edu>
Wed, 20 Apr 2022 17:13:59 +0000 (10:13 -0700)
committerMin-Yih Hsu <minyihh@uci.edu>
Wed, 4 May 2022 20:16:51 +0000 (13:16 -0700)
commit794c4218a64757dd1cdfb787262d6327000ac5cd
treeee2028ca9547febd4da79d04aa1883734f190e1a
parent655294866cf8fa719b240a7e9fe51de9a2c48ddc
[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
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
mlir/test/Target/LLVMIR/Import/incorrect-instmap-assignment.ll [new file with mode: 0644]