Currently, the instruction combiner is broken when it tries to eliminate a setcc with a cast and a constant as an argument under a number of constraints: 1. Only for setgt (>) and setlt (<) 2. Only when one of the operands is a case and the other is a constant 3. Only when the cast is to a larger integer type (e.g. sbyte -> int). The code in question is the switch case starting at InstructionCombining.cpp:2021. Currently, the optimization has been disabled (leading to performance degradation of generated code). The disablement was provided by this patch: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041101/020140.html This bug aims to fix the pessimization by correctly re-enabling the optimization.
Created attachment 182 [details] Assembly test case The obvious three line test case.
Created attachment 183 [details] IRC Log Describing The Problem
This testcase may work better: bool %nada(sbyte %X) { %Y = cast sbyte %X to uint %C = setgt uint %Y, 255 ret bool %C } Otherwise, instcombine will just delete all of the code. :) In this case, it should be optimized to: bool %nada(sbyte %X) { %C = setlt sbyte %X, 0 ret bool %C } -Chris
Test Case Is Here: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041122/021393.html
Created attachment 185 [details] Potential Patch I've run through this numerous times and can't see anything wrong with it even though it is substantially the same as the original.
Created attachment 186 [details] Test Case More rigorous test case
The "Potential Patch" produces the following xform from the "Test Case". I'd appreciate it if you could review this and let me know if its okay to commit or what I did wrong. bool %lt_signed_to_large_unsigned(sbyte %SB) { ret bool true } bool %lt_signed_to_large_signed(sbyte %SB) { ret bool true } bool %lt_signed_to_large_negative(sbyte %SB) { ret bool false } bool %lt_signed_to_small_unsigned(sbyte %SB) { %C = setlt sbyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_signed_to_small_signed(sbyte %SB) { %C = setlt sbyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_signed_to_small_negative(sbyte %SB) { %C = setlt sbyte %SB, -17 ; <bool> [#uses=1] ret bool %C } bool %lt_unsigned_to_large_unsigned(ubyte %SB) { ret bool true } bool %lt_unsigned_to_large_signed(ubyte %SB) { ret bool true } bool %lt_unsigned_to_large_negative(ubyte %SB) { ret bool false } bool %lt_unsigned_to_small_unsigned(ubyte %SB) { %C = setlt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_unsigned_to_small_signed(ubyte %SB) { %C = setlt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_unsigned_to_small_negative(ubyte %SB) { ret bool false } bool %gt_signed_to_large_unsigned(sbyte %SB) { %C = setgt sbyte %SB, -127 ; <bool> [#uses=1] ret bool %C } bool %gt_signed_to_large_signed(sbyte %SB) { ret bool false } bool %gt_signed_to_large_negative(sbyte %SB) { ret bool true } bool %gt_signed_to_small_unsigned(sbyte %SB) { %C = setgt sbyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_signed_to_small_signed(sbyte %SB) { %C = setgt sbyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_signed_to_small_negative(sbyte %SB) { %C = setgt sbyte %SB, -17 ; <bool> [#uses=1] ret bool %C } bool %gt_unsigned_to_large_unsigned(ubyte %SB) { ret bool false } bool %gt_unsigned_to_large_signed(ubyte %SB) { ret bool false } bool %gt_unsigned_to_large_negative(ubyte %SB) { ret bool true } bool %gt_unsigned_to_small_unsigned(ubyte %SB) { %C = setgt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_unsigned_to_small_signed(ubyte %SB) { %C = setgt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_unsigned_to_small_negative(ubyte %SB) { ret bool true }
Created attachment 187 [details] Second Potential Patch
Okay, the new patch now delivers: bool %lt_signed_to_large_unsigned(sbyte %SB) { ret bool true } bool %lt_signed_to_large_signed(sbyte %SB) { ret bool true } bool %lt_signed_to_large_negative(sbyte %SB) { ret bool false } bool %lt_signed_to_small_unsigned(sbyte %SB) { %Y = cast sbyte %SB to ubyte ; <ubyte> [#uses=1] %C = setlt ubyte %Y, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_signed_to_small_signed(sbyte %SB) { %C = setlt sbyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_signed_to_small_negative(sbyte %SB) { %C = setlt sbyte %SB, -17 ; <bool> [#uses=1] ret bool %C } bool %lt_unsigned_to_large_unsigned(ubyte %SB) { ret bool true } bool %lt_unsigned_to_large_signed(ubyte %SB) { ret bool true } bool %lt_unsigned_to_large_negative(ubyte %SB) { ret bool false } bool %lt_unsigned_to_small_unsigned(ubyte %SB) { %C = setlt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_unsigned_to_small_signed(ubyte %SB) { %C = setlt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %lt_unsigned_to_small_negative(ubyte %SB) { ret bool false } bool %gt_signed_to_large_unsigned(sbyte %SB) { %Y = cast sbyte %SB to ubyte ; <ubyte> [#uses=1] %C = setgt ubyte %Y, 129 ; <bool> [#uses=1] ret bool %C } bool %gt_signed_to_large_signed(sbyte %SB) { ret bool false } bool %gt_signed_to_large_negative(sbyte %SB) { ret bool true } bool %gt_signed_to_small_unsigned(sbyte %SB) { %Y = cast sbyte %SB to ubyte ; <ubyte> [#uses=1] %C = setgt ubyte %Y, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_signed_to_small_signed(sbyte %SB) { %C = setgt sbyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_signed_to_small_negative(sbyte %SB) { %C = setgt sbyte %SB, -17 ; <bool> [#uses=1] ret bool %C } bool %gt_unsigned_to_large_unsigned(ubyte %SB) { ret bool false } bool %gt_unsigned_to_large_signed(ubyte %SB) { ret bool false } bool %gt_unsigned_to_large_negative(ubyte %SB) { ret bool true } bool %gt_unsigned_to_small_unsigned(ubyte %SB) { %C = setgt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_unsigned_to_small_signed(ubyte %SB) { %C = setgt ubyte %SB, 17 ; <bool> [#uses=1] ret bool %C } bool %gt_unsigned_to_small_negative(ubyte %SB) { ret bool true }
Created attachment 188 [details] Third Potential Patch This is the same as the last one but it puts the guts of the Instruction::Cast case of visitSetCondInst into visitSetCondInstWithCastAndConstant
The last patch is currently awaiting results of nightly testing. It will be committed if it passes.
Resolution Patch Is Here: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041122/021408.html
Part of this patch was disabled here: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20050110/023419.html It miscompiles the testcase in the commit. -Chris
This transform was reimplemented and reenabled here: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20050418/025735.html -Chris
I just want to add a short note that the code contains several shifts in the form "(1ULL << C2->getType()->getPrimitiveSizeInBits())-1;" which _might_ cause shift overflows. Maybe it would be a good idea to implement some "uint64_t getHighBit(unsigned bits)" and "uint64_t generateMask(unsigned bits)" helper functions that correctly deal with bits == 64. Or these could also made member functions of Type.
Excellent point, I'll audit these. Thanks! -Chris
Yup, there were bugs. Fixed here: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20050418/025751.html Thanks! -Chris