[PATCH] D69120: [TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (2/2)

Joan LLuch via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 29 14:40:28 PDT 2019


joanlluch marked 4 inline comments as done.
joanlluch added inline comments.


================
Comment at: llvm/test/CodeGen/MSP430/shift-amount-threshold.ll:50
 
 define i16 @testExtendSignBit(i16 %a) {
 ; CHECK-LABEL: testExtendSignBit:
----------------
spatel wrote:
> spatel wrote:
> > Do we have test coverage for each code diff? 
> > It would be good to add a code comment for each test that describes the transform that we are trying to avoid.
> > For example, this one could copy the formula from the corresponding source code comment:
> >   // sext i1 (setgt iN X, -1) --> sra (not X), (N - 1)
> > 
> Also, the IR does not correspond exactly to that comment. It's fine to include tests with 'select', but if there is an intermediate transform that turns it into 'sext' or 'zext', then we should have a test that begins with that pattern in IR too if possible. We want to make sure that we do not accidentally bypass these changes with future patches.
I updated the test file as per your suggestion. Test cases are now focused on the individual transformations that we are trying to avoid. I also changed the IR to avoid intermediate transformations and focus on the pattern of interest.
Unfortunately, these changes implied some depart from the 'baseline' tests that were posted before, and thus the diff appears now a bit mixed,  but I think it is still informative enough to see the improvements.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69120/new/

https://reviews.llvm.org/D69120





More information about the llvm-commits mailing list