<div dir="ltr">Hi Andrea,<div><br></div><div>answers are inlined!</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-05 7:11 GMT-07:00 Andrea Di Biagio <span dir="ltr"><<a href="mailto:andrea.dibiagio@gmail.com" target="_blank">andrea.dibiagio@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Marcello,<br>
<br>
I like this new approach more than the previous one.<br>
I have some comments though (mostly style related). Please, can you<br>
have a look at them?<br>
<br>
//-----<br>
<br>
About patch "x86_opt_wrap_v4.patch"<br>
<br>
+; Function Attrs: nounwind readnone ssp uwtable<br>
+define i64 @fact2(i64 %x) #0 {<br>
<br>
Please remove the comment. Also, attribute group #0 is not defined!<br>
Please remove any reference to it.<br>
(on a different topic: It would be nice to have a warning for cases<br>
like this one where a non existing attribute group is referenced in<br>
the IR..).<br>
<br>
//-----<br></blockquote><div><br></div><div>Will do!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
About patch "flag_nodes_v4.patch"<br>
<br>
In include/llvm/CodeGen/SelectionDAG.h:<br>
<div class=""><br>
+ /// isBinOpWithFlags - Returns true if the opcode is a binary operation<br>
+ /// with flags.<br>
+ static bool isBinOpWithFlags(unsigned Opcode) {<br>
+ switch (Opcode) {<br>
</div>+ case BINOP_NODES_W_FLAGS: return true;<br>
<div class="">+ default: return false;<br>
+ }<br>
+ }<br>
</div>+<br>
<br>
I think you can move that definition in SelectionDAGNodes.h. You can<br>
simplify the code in method 'BinaryWithFlagsSDNode::classof' If you<br>
can make that definition visible. For example, method '<br>
BinaryWithFlagsSDNode ::classof' could be rewritten as follows:<br>
<br>
//<br>
static bool classof(const SDNode *N) {<br>
return isBinOpWithFlags(N->getOpcode());<br>
//<br>
<br>
Also, if possible, please remove macro BINOP_NODES_W_FLAGS. In my<br>
opinion it doesn't make the code more readable.<br>
<div class=""></div></blockquote><div><br></div><div>About that ... BINOP_NODES_W_FLAGS is used in a couple of places (in switches) where mainly I cannot avoid to have the case labels there (I would like to use isBinOpWithFlags() everywhere, but in switches is not possible and putting the check outside the switch would be less performant, so I prefer having the cases there.</div>
<div><br></div><div>Do you think just duplicating the cases in those couple of places would be ok?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""> <br></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
+ /// isBinOpWithFlags - Returns true if the opcode is a binary operation<br>
+ /// with flags.<br>
</div>You could use \brief here.<br>
<br>
In lib/CodeGen/SelectionDAG/DAGCombiner.cpp:<br>
<br>
-//===-- DAGCombiner.cpp - Implement a DAG node combiner<br>
-------------------===//<br>
+ //===-- DAGCombiner.cpp - Implement a DAG<br>
node combiner -------------------===//<br>
<br>
Please revert that change.<br>
<br></blockquote><div>*** blushes ***</div><div><br></div><div>:D Will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In method 'DAGCombiner::combine':<br>
<br>
+ if (isa<BinaryWithFlagsSDNode>(N)) {<br>
+ const BinaryWithFlagsSDNode *BinNode = cast<BinaryWithFlagsSDNode>(N);<br>
<br>
You can use 'dyn_cast' instead of 'isa' + 'cast'.<br></blockquote><div><br></div><div>Ok </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In SelectionDAG.cpp:<br>
<br>
Method SelectionDAG::GetBinarySDNode<br>
<br>
+ return FN;<br>
+ } else {<br>
+ BinarySDNode *N = new (NodeAllocator) BinarySDNode(Opcode, DL.getIROrder(),<br>
+ DL.getDebugLoc(), VTs,<br>
+ N1, N2);<br>
+ return N;<br>
+ }<br>
<br>
Don't use 'else' after a return.<br>
<br></blockquote><div>Ok </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In SelectionDAGBuilder.cpp:<br>
(methods SelectionDAGBuilder::visitShift and SelectionDAGBuilder::visitBinary)<br>
<br>
+ const OverflowingBinaryOperator *OFBinOp =<br>
+ dyn_cast<const OverflowingBinaryOperator>(&I);<br>
+ const PossiblyExactOperator *ExactOp =<br>
+ dyn_cast<const PossiblyExactOperator>(&I);<br>
+<br>
+ const bool IsOverflowBinOperator = OFBinOp != nullptr;<br>
+ const bool IsExactOperator = ExactOp != nullptr;<br>
+ const bool nuw = IsOverflowBinOperator ?<br>
OFBinOp->hasNoUnsignedWrap() : false;<br>
+ const bool nsw = IsOverflowBinOperator ? OFBinOp->hasNoSignedWrap() : false;<br>
+ const bool exact = IsExactOperator ? ExactOp->isExact() : false;<br>
<br>
Instead of using a dyn_cast + check against nullptr + ternary<br>
conditional operator,<br>
wouldn't be more readable if you write something like this?<br>
<br>
///////////<br>
bool nuw = false;<br>
bool nsw = false;<br>
bool exact = false;<br>
if (OverflowingBinaryOperator *OFBinOp =<br>
dyn_cast<const OverflowingBinaryOperator>(&I)) {<br>
nuw = OFBinOp->hasNoUnsignedWrap();<br>
nsw = OFBinOp->hasNoSignedWrap() ;<br>
}<br>
if (PossiblyExactOperator *ExactOp =<br>
dyn_cast<const PossiblyExactOperator>(&I))<br>
exact = ExactOp->isExact();<br>
//////////<br>
<br></blockquote><div>Seems nicer! Thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In your new test X86/2014-05-30-CombineAddNSW.ll:<br>
<br>
+; Function Attrs: nounwind readnone ssp uwtable<br>
+define i32 @foo(i32 %a, i32 %b) #0 {<br>
<br>
Please remove the comment and the reference to attribute group #0.<br>
Also, please add a brief comment explaining why you expect an 'add'<br>
instead of a 'xor' in this case.<br>
<br></blockquote><div><br></div><div>Will do. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks,<br>
Andrea<br>
</blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Cheers,</div><div class="gmail_extra">Marcello</div></div>