[LLVMdev] Multiple one-line bugs in LLVM

[Duncan Sands]

Hi Lockal S,

> ----
>
> lib/Target/X86/X86ISelLowering.cpp:11689
> !DAG.isKnownNeverZero(LHS)&&  !DAG.isKnownNeverZero(LHS))
>
> Note that there are identical subexpressions '!DAG.isKnownNeverZero (LHS)' to
> the left and to the right of the '&&' operator.
> The second subexpression should probably be !DAG.isKnownNeverZero(RHS)).

a patch fixing this was posted by Ivan Krasin.  It seems correct, but is waiting
on someone writing a testcase.

>
> ----
>
> lib/Transforms/Scalar/ObjCARC.cpp:1138
> void clearBottomUpPointers() {
>      PerPtrTopDown.clear();
> }
>
> void clearTopDownPointers() {
>      PerPtrTopDown.clear();
> }
>
> Note that the body of 'clearTopDownPointers' function is fully equivalent to
> the body of 'clearBottomUpPointers' function. The advised solution is to change
> this code into
>
> void clearBottomUpPointers() {
>      PerPtrBottomUp.clear();
> }

This is probably correct.  Hopefully John can comment.

>
> ----
>
> lib/Analysis/InstructionSimplify.cpp:1891
>      bool NUW = LBO->hasNoUnsignedWrap()&&  LBO->hasNoUnsignedWrap();
>
> There are identical sub-expressions 'LBO->hasNoUnsignedWrap()' to the left and
> to the right of the '&&' operator. Looks like the correct code is
>
>      bool NUW = LBO->hasNoUnsignedWrap()&&  RBO->hasNoUnsignedWrap();

That is correct.  Ivan sent a patch for this too and I applied it.

>
> ----
>
> lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1384
>    if (InputByteNo<  ByteValues.size()/2) {
>      if (ByteValues.size()-1-DestByteNo != InputByteNo)
>        return true;
>    } else {
>      if (ByteValues.size()-1-DestByteNo != InputByteNo)
>        return true;
>    }
>
> Note that 'then' and 'else' are the same. It can be a problem or can not.

I've CC'd Chris since he wrote this code.

>
> ----
>
> lib/Target/X86/InstPrinter/X86InstComments.cpp:208
>    case X86::VPERMILPSri:
>      DecodeVPERMILPSMask(4, MI->getOperand(2).getImm(),
>                          ShuffleMask);
>      Src1Name = getRegName(MI->getOperand(0).getReg());
>    case X86::VPERMILPSYri:
>      DecodeVPERMILPSMask(8, MI->getOperand(2).getImm(),
>                          ShuffleMask);
>      Src1Name = getRegName(MI->getOperand(0).getReg());
>
> The 'Src1Name' variable is assigned values twice successively. So break
> instruction should be at line 212.

I've added the missing "break".

>
> ----
>
> lib/MC/MCParser/AsmLexer.cpp:149
>    while (CurChar != '\n'&&  CurChar != '\n'&&  CurChar != EOF)
>
> There are identical sub-expressions to the left and to the right of the '&&'
> operator: CurChar != '\n'&&  CurChar != '\n'. The second expression should
> probably be CurChar != '\n'?

Chris also added this code, hopefully he will comment.

>
> ----
> lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:505
>      result |= (icmp_eq ? (FoldMskICmp_Mask_AllZeroes |
>                            FoldMskICmp_Mask_AllZeroes |
>                            FoldMskICmp_AMask_Mixed |
>                            FoldMskICmp_BMask_Mixed)
>                         : (FoldMskICmp_Mask_NotAllZeroes |
>                            FoldMskICmp_Mask_NotAllZeroes |
>                            FoldMskICmp_AMask_NotMixed |
>                            FoldMskICmp_BMask_NotMixed));
>
> There are identical sub-expressions 'FoldMskICmp_Mask_AllZeroes' and
> 'FoldMskICmp_Mask_NotAllZeroes' to the left and to the right of the
> '|'
> operator. Probably not a bug, just some duplicated code.

I've CC'd Owen who added this code.

>
> ----
>
> include/llvm/Target/TargetLowering.h:267
>        switch (getTypeAction(Context, VT)) {
>        case Legal:
>          return VT;
>        case Expand:
>
> Note that the values of different enum types (LegalizeAction and
> LegalizeTypeAction) are compared. This code works well because of the same order
> for enums, but it would be better to change this into
>
>        switch (getTypeAction(Context, VT)) {
>        case TypeLegal:
>          return VT;
>        case TypeExpand:

Yup, I've made this change.

> ----
>
> Thanks a lots for any fixes and answers.

Thanks for letting us know about these issues.

Ciao, Duncan.