agozillon wrote:

Hey, @jplehr notified me of this issue (thank you), I've had a little look at 
it and it's related to an issue this PR fixes: 
https://github.com/llvm/llvm-project/pull/94541 which you are currently a 
reviewer of ironically (as I use and alter slightly a piece of functionality 
you ported, so thank you very much for that)! A slightly funny convergence 
there :-) 

Currently when generating the kernel we try to replace uses of values with 
their appropriate kernel input argument, in certain cases these are Constant's 
which poses an issue as we can't replace them with the Input arguments which 
are themselves not Constant I believe. So we can only replace instructions, to 
do so we transform the Constants to Instructions where necessary, the current 
function that's in place (a small naive function from myself) will do this 
within the kernel to a small extent, it doesn't handle nesting's very well for 
example, and apparently IR this patch will produce in the test cases that are 
failing. However, the PR I have opened above uses the more robust and 
comprehensive piece of functionality you made/ported via the function 
`convertUsersOfConstantsToInstructions` which solves the issues encountered 
when this PR is landed alongside some other issues I encountered recently.

So two ways forward I can see, depending on how urgent/annoying it is for this 
PR to be stalled:
1.  We disable the offending tests that are breaking the buildbot until the PR 
that will fix them lands, which will allow you to commit this as soon as 
they're disabled without angering the buildbot
2. We hold off landing this until the PR that fixes the issues in the tests 
lands, I am unfortunately not sure how long this will take as it's been up for 
a while with a lack of reviewers (having a review from you @nikic would be 
excellent as well for the minor additions to the functionality you added), 
although, I can likely chase some people up to give it a look over as it's 
urgent. 

I don't mind which route we go down, if we opt to disable the tests, I can land 
a commit shortly to do so, unless either of you @nikic @jplehr would prefer to 
do so :-) 

https://github.com/llvm/llvm-project/pull/95126
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to