Illegal node introduced by DAGCombiner after legal phase

Hello,

I’m getting an instruction selection error because DAGCombiner is introducing an illegal node after the legalizeDAG phase. Basically this is what is going on:

  1. During legalization, BR_JT gets expanded introducing a (mul x, 2).
  2. After legalization (AfterLegalizeDAG), that (mul x, 2) is converted to an (shl x, 1).

However, that shl node introduced is illegal, and since my custom lowering code won’t run after this phase it gets into the instruction selector. I would like to know if this is really a bug or it has to be handled by the target code. I guess I could add a custom dagcombine hook for this node and revert it in my target, but I want to hear what is the best thing to do.

Thanks!

Hi Borja,

I'm getting an instruction selection error because DAGCombiner is introducing an
illegal node after the legalizeDAG phase. Basically this is what is going on:

1) During legalization, BR_JT gets expanded introducing a (mul x, 2).
2) After legalization (AfterLegalizeDAG), that (mul x, 2) is converted to an
(shl x, 1).

However, that shl node introduced is illegal, and since my custom lowering code
won't run after this phase it gets into the instruction selector. I would like
to know if this is really a bug or it has to be handled by the target code. I
guess I could add a custom dagcombine hook for this node and revert it in my
target, but I want to hear what is the best thing to do.

it sounds like a bug in the DAG combiner. I think checks for legal operations
tend to get added to the DAG combiner on an "as needed" basis, so maybe no-one
had a platform for which this shift is not legal before.

Ciao, Duncan.

Borja,

In this situation, you need to find the place where shl is generated and add a check to see if shl is legal before allowing it to do the transform.

Micah

Although I’m working on an out of tree target, this bug also happens for the MSP430 target since we both share the limitation that shifts have to be done in one bit at a time, that’s why we need custom lowering.
Micah, I don’t have commit access to fix it but the shl is getting generated in line 1770 of DAGCombiner.cpp (inside the visitMUL function) (fold (mul x, (1 << c)) → x << c).

But there’s something more about the design of how this works, if the dagcombiner can potentially add illegal nodes after legalization, some of these illegal nodes are going to be “cheaper” (shl vs mul) than not performing the combine at all so maybe the custom lowering code could be run again to catch these cases. I haven’t thought too much about this, but leaving a (mul x, 2) around doesn’t sound too right.

Thanks for both replies.