<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Nov 13, 2017, at 4:32 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" class="">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">On Nov 13, 2017, at 8:51 AM, Kristof Beyls via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""><br class="">kristof.beyls added inline comments.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:194-201<br class="">+ unsigned Op = MI.getOperand(i).getReg();<br class="">+ // G_UNMERGE_VALUES has variable number of operands, but there is only<br class="">+ // one source type and one destination type as all destinations must be the<br class="">+ // same type. So, get the last operand if TypeIdx == 1.<br class="">+ if (MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES && TypeIdx == 1) {<br class="">+ Op = MI.getOperand(MI.getNumOperands() - 1).getReg();<br class="">+ }<br class="">----------------<br class="">To make this easier to read, I think it may make sense to factor this out into a separate function, e.g. with a name like "getTyFromTypeIdx(MI, TypeIdx)". I'm not sure on the name, but doing it this way would make this code a lot easier to understand, I think.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:359<br class="">+ for (const auto &Ty :<br class="">+ {s32, s64, s96, s128, s192, v2s32, v3s32, v4s32, v2s64, v4s64, v6s64}) {<br class="">+ setAction({G_MERGE_VALUES, Ty}, Legal);<br class="">----------------<br class="">I haven't thought this through at all, but I'm a bit surprised v8s64 also isn't legal if v6s64 is?<br class="">Similarly for S256 if s192 is legal?<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">FWIW, I believe we should just specify the size (instead of full types) for operations that deal only with bit manipulation (merge_value, phi, bitwise operation, ld/st).</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; color: rgb(69, 69, 69);" class="">I think it’s good to legalize these operations based on full types because legalization actions might be different, especially for vector types. Otherwise, we’d need to mark it as Custom, check the full type and replace it with something else even if it’s legal.</div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; color: rgb(69, 69, 69); min-height: 14px;" class=""><br class=""></div><div style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; color: rgb(69, 69, 69);" class="">Volkan</div></div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><br class=""><br class=""><a href="https://reviews.llvm.org/D39823" class="">https://reviews.llvm.org/D39823</a></blockquote></div></blockquote></div><br class=""></body></html>