[PATCH] D103661: [IR] Add utility to convert constant expression operands (of an instruction) to instructions.

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added a comment.

In D103661#2798554 <https://reviews.llvm.org/D103661#2798554>, @foad wrote:

> Given a constant expression tree that contains CE, you are converting the entire tree to instructions. That is overkill. You only need to convert the nodes on the path from CE to the root.

Strictly speaking, yes, I agree, but, this requires to explicitly enumerate all possible paths from root to CE (since CE can possbly appear as more than one node in the same tree), and convert nodes only in those enumerated paths into instructions (please take a look at example that I gave while replying to one of @rampitec comments below). I am not handling it yet. Let's see how to handle it.



================
Comment at: llvm/lib/IR/ReplaceConstant.cpp:107
+
+    if (CE->getNumOperands()) {
+      SmallPtrSet<Value *, 4> Operands2;
----------------
rampitec wrote:
> To address Jay's comment I think you can pass an original CE into this function and amend the if to only execute it if CE != ReplacedCE. It will then skip the operands of the expression being replaced itself. I.e.:
> 
> 
> ```
> @@ -77,12 +77,13 @@ void convertConstantExprsToInstructions(Instruction *I, ConstantExpr *CE,
> 
>    // Convert constant expressions operands of I (from the set CEOperands) to
>    // instructions.
> -  convertConstantExprsToInstructions(I, CEOperands, Insts);
> +  convertConstantExprsToInstructions(I, CEOperands, Insts, CE);
>  }
> 
>  void convertConstantExprsToInstructions(Instruction *I,
>                                          SmallPtrSetImpl<Value *> &Operands,
> -                                        SmallPtrSetImpl<Instruction *> &Insts) {
> +                                        SmallPtrSetImpl<Instruction *> &Insts,
> +                                        ConstantExpr *C) {
>    for (Use &U : I->operands()) {
>      auto *V = U.get();
> 
> @@ -104,11 +105,9 @@ void convertConstantExprsToInstructions(Instruction *I,
>      I->replaceUsesOfWith(CE, NI);
>      Insts.insert(NI);
> 
> -    if (CE->getNumOperands()) {
> +    if (C != CE && CE->getNumOperands()) {
>       SmallPtrSet<Value *, 4> Operands2;
>       for (Use &UU : CE->operands())
>         Operands2.insert(UU.get());
> -      convertConstantExprsToInstructions(NI, Operands2, Insts);
> +      convertConstantExprsToInstructions(NI, Operands2, Insts, C);
>      }
> 
>      CE->removeDeadConstantUsers();
> ```
> Note there are never recursive constant expressions.
This will some extent reduces the unnecessary CE to I conversion. But, does not fully handle what @foad is suggesting. Consider below tree. 

```
                                     I0
                                     |
                                    CE1
                                  /     \
                               CE2      CE3
                              /   \        \
                           CE4    CE4      CE5
                           /  \      \
                        CE6   CE6    CE7
                         /      \
                      CE8       CE9
```

Assuminng that, we need to convert CE4 into instructions, we only need to touch CEs in the two paths - (1) CE1, CE2, CE4 and (2) CE1, CE2, CE4.  So result would be something like below.

```
I4_2 (CE7)
I4_1 (CE6, CE6)
I2 (I4_1, I4_2)
I1 (I2,  CE3)
I0 (I1)
```

But, the above code that you are suggesting here will convert every sub-tree except sub-trees starting from CE4.

Let's see what to do about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103661