<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Sanjin<div class=""><br class=""></div><div class="">I’m not sure if there’s a better way to handle this, but i have some general comments on the style in the patch.</div><div class=""><br class=""></div><div class="">+ int MatchingId; // Same Id is set for corresponding load/store intrinsics.</div><div class=""><br class=""></div><div class="">This should be an enum instead of an integer. Same goes for all other definitions and uses of this variable.</div><div class=""><br class=""></div><div class=""><div class="">+ /// \returns An instruction defining the result of a memory intrinsic. New</div><div class="">+ /// instructions may be created to extract the result from an intrinsic memory</div><div class="">+ /// operation.</div><div class="">+ virtual Value* getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,</div><div class=""><br class=""></div><div class="">You should mention that null is also a valid return value, if it was unable to get a valid intrinsic.</div><div class=""><br class=""></div><div class="">+bool AArch64TTI::getTgtMemIntrinsic</div><div class="">+ (IntrinsicInst* Inst, MemIntrinsicInfo& Info) const {</div><div class=""><br class=""></div><div class="">…</div><div class=""><br class=""></div><div class=""> +Value*</div><div class="">+TargetTransformInfo::getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,</div><div class="">+ Type *ExpectedType)</div><div class="">+ const {</div><div class=""><br class=""></div><div class="">Please run clang-format over the patch to see if there’s a better way to format long lines like these.</div><div class=""><br class=""></div><div class=""><div class="">+Value* AArch64TTI::getOrCreateResultFromMemIntrinsic(IntrinsicInst* Inst,</div><div class="">+ Type* ExpectedType) const {</div><div class="">+ Value* Res = nullptr;</div><div class="">+ switch (Inst->getIntrinsicID()) {</div><div class="">+ default:</div><div class="">+ break;</div></div><div class="">...</div><div class="">+ return Res;</div><div class=""><br class=""></div><div class="">I would just return nullptr from the default case here. In fact i’d not have ‘Res’ at all in this method and just return from where you need to.</div><div class=""><div class=""><br class=""></div><div class="">+ case Intrinsic::aarch64_neon_st4:</div><div class="">+ // Create a struct type</div><div class="">+ StructType *ST = dyn_cast<StructType>(ExpectedType);</div></div><div class=""><br class=""></div><div class="">You should have { } over the body of this case as you’ve defined a variable here which is going to potentially break if more cases add their own variables.</div><div class=""><br class=""></div><div class=""><div class="">+ // Wrapper class to handle memory instructions, including loads, stores and</div><div class="">+ // intrinsic loads and stores defined by the target.</div><div class="">+ class ParseMemoryInst {</div></div><div class=""><br class=""></div><div class="">/// and \brief to have the style of your other code.</div><div class=""><br class=""></div><div class="">+ if (TTI->getTgtMemIntrinsic(II, Info)) {</div><div class="">+ if (Info.NumMemRefs == 1) {</div><div class="">+ Store = Info.WriteMem;</div><div class="">+ Load = Info.ReadMem;</div><div class="">+ MayReadFromMemory = Info.ReadMem;</div><div class="">+ MayWriteToMemory = Info.WriteMem;</div><div class="">+ Vol = Info.Vol;</div><div class="">+ Ptr = Info.PtrVal;</div><div class="">+ }</div><div class="">+ }</div><div class=""><br class=""></div><div class="">You can reduce nesting with 'if (!TTI->getTgtMemIntrinsic(II, Info)) return'</div><div class=""><br class=""></div><div class=""><div class="">+ ParseMemoryInst MemInst(Inst, TTI);</div><div class=""> // If this is a non-volatile load, process it.</div><div class="">- if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {</div><div class="">+ if (MemInst.isLoad()) {</div></div><div class=""><br class=""></div><div class="">I think here you need to first check if MemInst was constructed before you can safely check isLoad. You may want to add an isValid method and change it to ‘ MemInst.isValid() && MemInst.isLoad()’. Probably similarly for later uses. Alternatively you could override bool operator() instead of defining isValid().</div><div class=""><br class=""></div><div class=""><div class="">- if (Inst->mayReadFromMemory())</div><div class="">+ if (MemInst.mayReadFromMemory())</div></div><div class=""><br class=""></div><div class="">This isn’t a valid change as the implementation of Inst->mayReadFromMemory() handles things like Instruction::VAArg which ParseMemoryInst doesn’t.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Pete</div><div><blockquote type="cite" class=""><div class="">On Jan 16, 2015, at 12:28 PM, Sanjin Sijaric <<a href="mailto:ssijaric@codeaurora.org" class="">ssijaric@codeaurora.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="WordSection1" style="page: WordSection1; font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Hi,<o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">We have a need to common target specific load/store intrinsics (in our case, AArch64 intrinsics). This patch handles commoning of a few of these intrinsics in Early CSE. We modified the Early CSE a bit to get the necessary information from the target. Is there a more appropriate place to handle this other than Early CSE, or a better way to handle it in Early CSE?<o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Thanks,<o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Sanjin<o:p class=""></o:p></div></div><span id="cid:F2CD03F4-C2F1-42C3-97BA-57278941C140@apple.com"><CSEPATCH></span><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">llvm-commits mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">llvm-commits@cs.uiuc.edu</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="color: purple; text-decoration: underline; font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br class=""></div></body></html>