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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added inline comments.


================
Comment at: llvm/lib/IR/ReplaceConstant.cpp:107
+
+    if (CE->getNumOperands()) {
+      SmallPtrSet<Value *, 4> Operands2;
----------------
hsmhsm wrote:
> 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.
The above tree is incorrect, please read it as below.

```
                                     I0
                                     |
                                    CE1
                                  /     \
                               CE2      CE3
                              /   \        \
                           CE4    CE4      CE5
                              \      \
                              CE7    CE7                         
                      
```

and the CE to I conversion as below:

```
I4 (CE7)
I2 (I4, I4)
I1 (I2,  CE3)
I0 (I1)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103661