[PATCH] D30611: [x86] don't blindly transform SETB into SBB

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34117
+    SDValue SBB = materializeSBB(Y.getNode(), Y.getOperand(1), DAG);
+    if (SBB.getValueSizeInBits() != VT.getSizeInBits())
+      SBB = DAG.getZExtOrTrunc(SBB, DL, VT);
----------------
RKSimon wrote:
> ScalarSizeInBits - or is that being too pessimistic?
Not sure how there could be a difference: materializeSBB() only works with i1 or i8 (there's an assert in there). So I don't think there's any chance of vectors here at the moment. Did I understand the comment?


================
Comment at: test/CodeGen/X86/add-of-carry.ll:3
 ; RUN: llc < %s -mtriple=i686-unknown-unknown | FileCheck %s
 
 ; <rdar://problem/8449754>
----------------
RKSimon wrote:
> The changes in this file don't look related to the patch.
I tried to explain this in the summary - maybe not well enough though. :)
I'd like to have a record of where this transform is still firing along with this patch, so we understand what patterns are actually helped by adc/sbb (in theory). I guess a test comment would do a better job than these phantom diffs. I'll change that. 


================
Comment at: test/CodeGen/X86/sse42-intrinsics-x86.ll:98
 
-define i32 @test_x86_sse42_pcmpestric128(<16 x i8> %a0, <16 x i8> %a2) {
+define i32 @test_x86_sse42_pcmpestric128(<16 x i8> %a0, <16 x i8> %a2) nounwind {
 ; SSE42-LABEL: test_x86_sse42_pcmpestric128:
----------------
RKSimon wrote:
> The stack usage is unfortunate.....
Agreed - not sure where that goes wrong yet.


https://reviews.llvm.org/D30611