Question about DAGCombiner::MatchRotate function

Hi all,

While I test "clang-tests/gcc-4_2-testsuite/src/gcc.c-torture/execute/20020226-1.c", I faced something wrong with "DAGCombiner::MatchRotate" function.

This function tries to consume some patterns and generate "ROTL" or "ROTR" dag node as following comments:

"DAGCombier::MatchRotate" function in DAGCombiner.cpp
Pattern1
// fold (or (shl (*ext x), (*ext y)),
// (srl (*ext x), (*ext (sub 32, y)))) ->
// (*ext (rotl x, y))
// fold (or (shl (*ext x), (*ext y)),
// (srl (*ext x), (*ext (sub 32, y)))) ->
// (*ext (rotr x, (sub 32, y)))

pattern2
// fold (or (shl (*ext x), (*ext (sub 32, y))),
// (srl (*ext x), (*ext y))) ->
// (*ext (rotl x, y))
// fold (or (shl (*ext x), (*ext (sub 32, y))),
// (srl (*ext x), (*ext y))) ->
// (*ext (rotr x, (sub 32, y)))

In order to do this, code checks whether target supports this dag operation with specific type(LHS's type) through "TLI.isOperationLegalOrCustom" function. I guess the reason is following "LegalizeType" can not promote "ROTL" and "ROTR".

The problem is that code does not check this with new specific type "LArgVT" and "RArgVT" while processing above patterns. How do you think about this? I think checking code is needed. I have attached simple patch to fix it. Please review this.

Thanks,
JinGu Kang

patch.txt (2.39 KB)

Hi JinGu,

It's normally best to send patches to the llvm-commits list. That's
where most of the reviews happen.

The problem is that code does not check this with new specific type "LArgVT"
and "RArgVT" while processing above patterns. How do you think about this? I
think checking code is needed.

I agree.

I have attached simple patch to fix it. Please review this.

Which target does this affect (that you know of)? It would be really
good to have a test (like the other .ll files in test/CodeGen/XXX) for
this in the patch.

Cheers.

Tim.

Hi Tim,

I appreciate your response. I faced this error with new architecture so I have no idea about correct working example with existing architectures on llvm at this moment. Because some patterns can be disappeared or changed according to architectures before this dag combining. I am sorry for that. If I find a working example, I will commit this. And I will send a e-mail to llvm-commits next time. :slight_smile:

Thanks for your kind response,
JinGu Kang